From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Furquan Shaikh <furquan@google.com>
Cc: Dan Carpenter <dan.carpenter@oracle.com>,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
adurbin@google.com
Subject: Re: [PATCH] drivers: input: Use single i2c_transfer transaction when using RM_CMD_BANK_SWITCH
Date: Thu, 20 Aug 2020 15:45:51 -0700 [thread overview]
Message-ID: <20200820224551.GX1665100@dtor-ws> (raw)
In-Reply-To: <20200818234215.2255611-1-furquan@google.com>
Hi Furquan,
On Tue, Aug 18, 2020 at 04:42:15PM -0700, Furquan Shaikh wrote:
> On an AMD chromebook, where the same I2C bus is shared by both Raydium
> touchscreen and a trackpad device, it is observed that interleaving of
> I2C messages when raydium_i2c_read_message() is called leads to the
> Raydium touch IC reporting incorrect information. This is the sequence
> that was observed to result in the above issue:
>
> * I2C write to Raydium device for RM_CMD_BANK_SWITCH
> * I2C write to trackpad device
> * I2C read from trackpad device
> * I2C write to Raydium device for setting address
> * I2C read from Raydium device >>>> This provides incorrect
> information
>
> This change updates raydium_i2c_read_message and
> raydium_i2c_send_message to perform all the I2C transfers in the
> function as part of a single i2c_transfer transaction. This ensures
> that no transactions are initiated to any other device on the same bus
> in between and hence the information obtained from the touchscreen
> device is correct. Verified with the patch across multiple
> reboots (>100) that the information reported by the Raydium
> touchscreen device during probe is correct.
>
> Signed-off-by: Furquan Shaikh <furquan@google.com>
> ---
> drivers/input/touchscreen/raydium_i2c_ts.c | 102 ++++++++++++++++-----
> 1 file changed, 80 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/input/touchscreen/raydium_i2c_ts.c b/drivers/input/touchscreen/raydium_i2c_ts.c
> index fe245439adee..11c00d341eb1 100644
> --- a/drivers/input/touchscreen/raydium_i2c_ts.c
> +++ b/drivers/input/touchscreen/raydium_i2c_ts.c
> @@ -111,6 +111,15 @@ struct raydium_info {
> u8 y_res; /* units/mm */
> };
>
> +/*
> + * Header to be sent for RM_CMD_BANK_SWITCH command. This is used by
> + * raydium_i2c_{read|send}_message below.
> + */
> +struct __packed raydium_bank_switch_header {
> + u8 cmd;
> + __be32 be_addr;
> +};
I believe the preferred placement of __packed attribute is after the
definition:
dtor@dtor-ws:~/kernel/work $ git grep "struct __packed" | wc -l
210
dtor@dtor-ws:~/kernel/work $ git grep "} __packed" | wc -l
8718
> +
> /* struct raydium_data - represents state of Raydium touchscreen device */
> struct raydium_data {
> struct i2c_client *client;
> @@ -198,22 +207,44 @@ static int raydium_i2c_read(struct i2c_client *client,
> static int raydium_i2c_read_message(struct i2c_client *client,
> u32 addr, void *data, size_t len)
> {
> - __be32 be_addr;
> - size_t xfer_len;
> - int error;
> + int ret;
>
> while (len) {
> - xfer_len = min_t(size_t, len, RM_MAX_READ_SIZE);
> -
> - be_addr = cpu_to_be32(addr);
> + u8 read_addr = addr & 0xff;
> + struct raydium_bank_switch_header header = {
> + .cmd = RM_CMD_BANK_SWITCH,
> + .be_addr = cpu_to_be32(addr),
> + };
> + size_t xfer_len = min_t(size_t, len, RM_MAX_READ_SIZE);
> + /*
> + * Perform as a single i2c_transfer transaction to ensure that
> + * no other I2C transactions are initiated on the bus to any
> + * other device in between. Initiating transacations to other
> + * devices after RM_CMD_BANK_SWITCH is sent is known to cause
> + * read issues.
> + */
> + struct i2c_msg xfer[] = {
> + {
> + .addr = client->addr,
> + .len = sizeof(header),
> + .buf = (u8 *)&header,
> + },
> + {
> + .addr = client->addr,
> + .len = 1,
> + .buf = &read_addr,
> + },
> + {
> + .addr = client->addr,
> + .flags = I2C_M_RD,
> + .len = xfer_len,
> + .buf = data,
> + }
> + };
I think this can be moved out of while loop.
I also wonder if this can be actually combined with raydium_i2c_read().
As far as I understand read/writes to register above 255 require
page select write, so we can always prepare the header and then submit
either 3 or 2 messages in the transfer depending on the register we are
dealing with. Or maybe convert to regmap?
Thanks.
--
Dmitry
next prev parent reply other threads:[~2020-08-20 22:45 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-18 23:42 [PATCH] drivers: input: Use single i2c_transfer transaction when using RM_CMD_BANK_SWITCH Furquan Shaikh
2020-08-19 0:08 ` Dmitry Torokhov
2020-08-19 17:51 ` Furquan Shaikh
2020-08-20 22:45 ` Dmitry Torokhov [this message]
2020-08-21 2:40 ` [PATCH v2] " Furquan Shaikh
2020-09-09 0:44 ` Dmitry Torokhov
2020-09-14 6:04 ` Furquan Shaikh
2020-09-14 6:34 ` Dmitry Torokhov
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=20200820224551.GX1665100@dtor-ws \
--to=dmitry.torokhov@gmail.com \
--cc=adurbin@google.com \
--cc=dan.carpenter@oracle.com \
--cc=furquan@google.com \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/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).