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 <[email protected]>
>
> ---
> 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;
> }
>
> -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.
> +}
> +
> +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.
I can fix it up locally if you agree.
Thanks.
--
Dmitry
Hi Dmitry,
On Tue, Sep 8, 2020 at 5:44 PM Dmitry Torokhov
<[email protected]> 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 <[email protected]>
> >
> > ---
> > 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!
- Furquan
>
> Thanks.
>
> --
> Dmitry