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 Mailing List <linux-kernel@vger.kernel.org>,
Aaron Durbin <adurbin@google.com>
Subject: Re: [PATCH v2] drivers: input: Use single i2c_transfer transaction when using RM_CMD_BANK_SWITCH
Date: Sun, 13 Sep 2020 23:34:07 -0700 [thread overview]
Message-ID: <20200914063407.GP1665100@dtor-ws> (raw)
In-Reply-To: <CAEGmHFEC+873xzoG8eav1mHJk6mShknr2YeDWc5DwsviHoeYAw@mail.gmail.com>
On Sun, Sep 13, 2020 at 11:04:48PM -0700, Furquan Shaikh wrote:
> Hi Dmitry,
>
> On Tue, Sep 8, 2020 at 5:44 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Hi Furquan,
> >
> > On Thu, Aug 20, 2020 at 07:40:06PM -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 adds a new helper function `raydium_i2c_xfer()` that
> > > performs I2C transactions to the Raydium device. It uses the register
> > > address to decide if RM_CMD_BANK_SWITCH header needs to be sent to the
> > > device (i.e. if register address is greater than 255, then bank switch
> > > header is sent before the rest of the transaction). Additionally, it
> > > ensures that all the I2C operations performed as part of
> > > `raydium_i2c_xfer()` are done as a single i2c_transfer. This
> > > guarantees that no other transactions are initiated to any other
> > > device on the same bus in between. Additionally,
> > > `raydium_i2c_{send|read}*` functions are refactored to use this new
> > > helper function.
> > >
> > > 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>
> > >
> > > ---
> > > v2: Added a new helper function raydium_i2c_xfer so that all read and
> > > send transfers are handled using the same path. This helper function
> > > uses register address > 255 to decide whether to send
> > > RM_CMD_BANK_SWITCH command. Additionally, __packed keyword is moved
> > > to be placed after the structure defintion.
> > >
> > > drivers/input/touchscreen/raydium_i2c_ts.c | 132 +++++++++------------
> > > 1 file changed, 58 insertions(+), 74 deletions(-)
> > >
> > > diff --git a/drivers/input/touchscreen/raydium_i2c_ts.c b/drivers/input/touchscreen/raydium_i2c_ts.c
> > > index fe245439adee..261e4a290836 100644
> > > --- a/drivers/input/touchscreen/raydium_i2c_ts.c
> > > +++ b/drivers/input/touchscreen/raydium_i2c_ts.c
> > > @@ -51,6 +51,7 @@
> > >
> > > /* Touch relative info */
> > > #define RM_MAX_RETRIES 3
> > > +#define RM_RETRY_DELAY_MS 20
> > > #define RM_MAX_TOUCH_NUM 10
> > > #define RM_BOOT_DELAY_MS 100
> > >
> > > @@ -136,114 +137,98 @@ struct raydium_data {
> > > bool wake_irq_enabled;
> > > };
> > >
> > > -static int raydium_i2c_send(struct i2c_client *client,
> > > - u8 addr, const void *data, size_t len)
> > > +static int raydium_i2c_xfer(struct i2c_client *client, u32 addr, void *data,
> > > + size_t len, bool is_read)
> > > {
> > > - u8 *buf;
> > > - int tries = 0;
> > > - int ret;
> > > -
> > > - buf = kmalloc(len + 1, GFP_KERNEL);
> > > - if (!buf)
> > > - return -ENOMEM;
> > > -
> > > - buf[0] = addr;
> > > - memcpy(buf + 1, data, len);
> > > -
> > > - do {
> > > - ret = i2c_master_send(client, buf, len + 1);
> > > - if (likely(ret == len + 1))
> > > - break;
> > > -
> > > - msleep(20);
> > > - } while (++tries < RM_MAX_RETRIES);
> > > -
> > > - kfree(buf);
> > > -
> > > - if (unlikely(ret != len + 1)) {
> > > - if (ret >= 0)
> > > - ret = -EIO;
> > > - dev_err(&client->dev, "%s failed: %d\n", __func__, ret);
> > > - return ret;
> > > - }
> > > + struct raydium_bank_switch_header {
> > > + u8 cmd;
> > > + __be32 be_addr;
> > > + } __packed header = {
> > > + .cmd = RM_CMD_BANK_SWITCH,
> > > + .be_addr = cpu_to_be32(addr),
> > > + };
> > >
> > > - return 0;
> > > -}
> > > + u8 reg_addr = addr & 0xff;
> > >
> > > -static int raydium_i2c_read(struct i2c_client *client,
> > > - u8 addr, void *data, size_t len)
> > > -{
> > > struct i2c_msg xfer[] = {
> > > + {
> > > + .addr = client->addr,
> > > + .len = sizeof(header),
> > > + .buf = (u8 *)&header,
> > > + },
> > > {
> > > .addr = client->addr,
> > > .len = 1,
> > > - .buf = &addr,
> > > + .buf = ®_addr,
> > > },
> > > {
> > > .addr = client->addr,
> > > - .flags = I2C_M_RD,
> > > .len = len,
> > > .buf = data,
> > > + .flags = is_read ? I2C_M_RD : 0,
> > > }
> > > };
> > > +
> > > + /*
> > > + * If address is greater than 255, then RM_CMD_BANK_SWITCH needs to be
> > > + * sent first. Else, skip the header i.e. xfer[0].
> > > + */
> > > + int xfer_start_idx = (addr > 0xff) ? 0 : 1;
> > > + size_t xfer_count = ARRAY_SIZE(xfer) - xfer_start_idx;
> > > int ret;
> > >
> > > - ret = i2c_transfer(client->adapter, xfer, ARRAY_SIZE(xfer));
> > > - if (unlikely(ret != ARRAY_SIZE(xfer)))
> > > - return ret < 0 ? ret : -EIO;
> > > + ret = i2c_transfer(client->adapter, &xfer[xfer_start_idx], xfer_count);
> > > + if (likely(ret == xfer_count))
> > > + return 0;
> > >
> > > - return 0;
> > > + return -EIO;
> >
> > We are losing real error here, I'll change this to
> >
> > return ret < 0 ? ret : -EIO;
> >
> > > }
>
> Looks good.
>
> > >
> > > -static int raydium_i2c_read_message(struct i2c_client *client,
> > > - u32 addr, void *data, size_t len)
> > > +static int raydium_i2c_send(struct i2c_client *client, u32 addr,
> > > + const void *data, size_t len)
> > > {
> > > - __be32 be_addr;
> > > + int tries = 0;
> > > + int ret;
> > > +
> > > + do {
> > > + ret = raydium_i2c_xfer(client, addr, (void *)data, len, false);
> > > + if (likely(ret == 0))
> > > + return 0;
> > > +
> > > + msleep(RM_RETRY_DELAY_MS);
> > > + } while (++tries < RM_MAX_RETRIES);
> > > +
> > > + dev_err(&client->dev, "%s failed\n", __func__);
> > > + return -EIO;
> >
> > Again losing real error here, I'll rename ret to error and do
> >
> > return error;
> >
> > here.
>
> Looks good.
>
> >
> > > +}
> > > +
> > > +static int raydium_i2c_read(struct i2c_client *client, u32 addr, void *data,
> > > + size_t len)
> > > +{
> > > + int ret;
> > > size_t xfer_len;
> > > - int error;
> > >
> > > while (len) {
> > > xfer_len = min_t(size_t, len, RM_MAX_READ_SIZE);
> > > + ret = raydium_i2c_xfer(client, addr, data, len, true);
> >
> > I think this needs to be xfer_len, not len.
>
> Good catch. Yes, this needs to be xfer_len.
>
> >
> > I can fix it up locally if you agree.
>
> The suggestions look good to me. Thank you!
Great, applied.
--
Dmitry
prev parent reply other threads:[~2020-09-14 6:34 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
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 [this message]
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=20200914063407.GP1665100@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).