* [PATCH] drivers: input: Use single i2c_transfer transaction when using RM_CMD_BANK_SWITCH @ 2020-08-18 23:42 Furquan Shaikh 2020-08-19 0:08 ` Dmitry Torokhov 2020-08-20 22:45 ` Dmitry Torokhov 0 siblings, 2 replies; 8+ messages in thread From: Furquan Shaikh @ 2020-08-18 23:42 UTC (permalink / raw) To: Dmitry Torokhov Cc: Dan Carpenter, linux-input, linux-kernel, adurbin, Furquan Shaikh 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; +}; + /* 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, + } + }; - error = raydium_i2c_send(client, RM_CMD_BANK_SWITCH, - &be_addr, sizeof(be_addr)); - if (!error) - error = raydium_i2c_read(client, addr & 0xff, - data, xfer_len); - if (error) - return error; + ret = i2c_transfer(client->adapter, xfer, ARRAY_SIZE(xfer)); + if (ret != ARRAY_SIZE(xfer)) + return ret < 0 ? ret : -EIO; len -= xfer_len; data += xfer_len; @@ -224,22 +255,49 @@ static int raydium_i2c_read_message(struct i2c_client *client, } static int raydium_i2c_send_message(struct i2c_client *client, - u32 addr, const void *data, size_t len) + u32 addr, void *data, size_t len) { - __be32 be_addr = cpu_to_be32(addr); - int error; + int tries = 0; - error = raydium_i2c_send(client, RM_CMD_BANK_SWITCH, - &be_addr, sizeof(be_addr)); - if (!error) - error = raydium_i2c_send(client, addr & 0xff, data, len); + do { + int ret; + u8 write_addr = addr & 0xff; + struct raydium_bank_switch_header header = { + .cmd = RM_CMD_BANK_SWITCH, + .be_addr = cpu_to_be32(addr), + }; + + struct i2c_msg xfer[] = { + { + .addr = client->addr, + .len = sizeof(header), + .buf = (u8 *)&header, + }, + { + .addr = client->addr, + .len = 1, + .buf = &write_addr, + }, + { + .addr = client->addr, + .len = len, + .buf = data, + } + }; - return error; + ret = i2c_transfer(client->adapter, xfer, ARRAY_SIZE(xfer)); + if (likely(ret == ARRAY_SIZE(xfer))) + return 0; + + msleep(20); + } while (++tries < RM_MAX_RETRIES); + + return -EIO; } static int raydium_i2c_sw_reset(struct i2c_client *client) { - const u8 soft_rst_cmd = 0x01; + u8 soft_rst_cmd = 0x01; int error; error = raydium_i2c_send_message(client, RM_RESET_MSG_ADDR, -- 2.28.0.220.ged08abb693-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers: input: Use single i2c_transfer transaction when using RM_CMD_BANK_SWITCH 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 1 sibling, 1 reply; 8+ messages in thread From: Dmitry Torokhov @ 2020-08-19 0:08 UTC (permalink / raw) To: Furquan Shaikh; +Cc: Dan Carpenter, linux-input, linux-kernel, adurbin 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. The devices (touchpad and touchscreen) have to have different addresses and therefore should be able to operate independently of each other. Are you sure that the problem is not in i2c controller implementation that mixes up data streams from 2 separate devices? Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers: input: Use single i2c_transfer transaction when using RM_CMD_BANK_SWITCH 2020-08-19 0:08 ` Dmitry Torokhov @ 2020-08-19 17:51 ` Furquan Shaikh 0 siblings, 0 replies; 8+ messages in thread From: Furquan Shaikh @ 2020-08-19 17:51 UTC (permalink / raw) To: Dmitry Torokhov Cc: Dan Carpenter, linux-input, Linux Kernel Mailing List, Aaron Durbin Hello Dmitry, On Tue, Aug 18, 2020 at 5:08 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > 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. > > The devices (touchpad and touchscreen) have to have different addresses > and therefore should be able to operate independently of each other. Are > you sure that the problem is not in i2c controller implementation that > mixes up data streams from 2 separate devices? Yes, the I2C addresses for the touchpad and touchscreen devices are different on this platform. Based on i2c tracing and the operations performed by the driver, the transactions are being sent out to the correct address. The issue occurs only when a transaction for the touchpad device is sent after the `RM_CMD_BANK_SWITCH` command is sent to the touchscreen device and before the driver reads back the required data. Snippet of tracing in good and bad case (with comments): Bad case: ``` i2c_write: i2c-0 #0 a=039 f=0000 l=5 [aa-20-00-1b-94] <<<<<< RM_CMD_BANK_SWITCH command to touchscreen device i2c_result: i2c-0 n=1 ret=1 i2c_write: i2c-0 #0 a=015 f=0000 l=2 [03-01] <<<<<< Transaction to touchpad device i2c_read: i2c-0 #1 a=015 f=0001 l=2 i2c_reply: i2c-0 #1 a=015 f=0001 l=2 [00-10] i2c_result: i2c-0 n=2 ret=2 i2c_write: i2c-0 #0 a=039 f=0000 l=1 [94] <<<<<< Write/read after RM_CMD_BANK_SWITCH to touchscreen device i2c_read: i2c-0 #1 a=039 f=0001 l=16 i2c_reply: i2c-0 #1 a=039 f=0001 l=16 [15-00-20-74-07-00-d8-02-00-00-00-ef-ff-ff-ff-00] <<<<<< Incorrect data i2c_result: i2c-0 n=2 ret=2 ``` Good case: ``` i2c_write: i2c-0 #0 a=015 f=0000 l=2 [03-01] <<<<<< Same transaction as above to touchpad device with same response from touchpad device i2c_read: i2c-0 #1 a=015 f=0001 l=2 i2c_reply: i2c-0 #1 a=015 f=0001 l=2 [00-10] i2c_result: i2c-0 n=2 ret=2 ... i2c_write: i2c-0 #0 a=039 f=0000 l=5 [aa-20-00-1b-94] <<<<<< RM_CMD_BANK_SWITCH command to touchscreen device i2c_result: i2c-0 n=1 ret=1 i2c_write: i2c-0 #0 a=039 f=0000 l=1 [94] <<<<<< Write/read after RM_CMD_BANK_SWITCH to touchscreen device i2c_read: i2c-0 #1 a=039 f=0001 l=16 i2c_reply: i2c-0 #1 a=039 f=0001 l=16 [42-18-22-a2-01-01-01-00-4a-2a-80-07-38-04-18-18] <<<<<< Correct data ``` It can be seen that the transaction which got interleaved in the bad case is also sent out in the good case with the same response from the touchpad device. Based on this evidence, the issue points towards a problem with touchscreen firmware expectations. Also, this same designware I2C driver is used on many other platforms as well. What is unique about this platform is that the touchpad and touchscreen devices share the same bus. Thanks, Furquan > > Thanks. > > -- > Dmitry ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers: input: Use single i2c_transfer transaction when using RM_CMD_BANK_SWITCH 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-20 22:45 ` Dmitry Torokhov 2020-08-21 2:40 ` [PATCH v2] " Furquan Shaikh 1 sibling, 1 reply; 8+ messages in thread From: Dmitry Torokhov @ 2020-08-20 22:45 UTC (permalink / raw) To: Furquan Shaikh; +Cc: Dan Carpenter, linux-input, linux-kernel, adurbin 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] drivers: input: Use single i2c_transfer transaction when using RM_CMD_BANK_SWITCH 2020-08-20 22:45 ` Dmitry Torokhov @ 2020-08-21 2:40 ` Furquan Shaikh 2020-09-09 0:44 ` Dmitry Torokhov 0 siblings, 1 reply; 8+ messages in thread From: Furquan Shaikh @ 2020-08-21 2:40 UTC (permalink / raw) To: Dmitry Torokhov Cc: Dan Carpenter, linux-input, linux-kernel, adurbin, Furquan Shaikh 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; } -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; +} + +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); - be_addr = cpu_to_be32(addr); - - error = raydium_i2c_send(client, RM_CMD_BANK_SWITCH, - &be_addr, sizeof(be_addr)); - if (!error) - error = raydium_i2c_read(client, addr & 0xff, - data, xfer_len); - if (error) - return error; + if (unlikely(ret != 0)) + return ret; len -= xfer_len; data += xfer_len; addr += xfer_len; } - return 0; } -static int raydium_i2c_send_message(struct i2c_client *client, - u32 addr, const void *data, size_t len) -{ - __be32 be_addr = cpu_to_be32(addr); - int error; - - error = raydium_i2c_send(client, RM_CMD_BANK_SWITCH, - &be_addr, sizeof(be_addr)); - if (!error) - error = raydium_i2c_send(client, addr & 0xff, data, len); - - return error; -} - static int raydium_i2c_sw_reset(struct i2c_client *client) { const u8 soft_rst_cmd = 0x01; int error; - error = raydium_i2c_send_message(client, RM_RESET_MSG_ADDR, - &soft_rst_cmd, sizeof(soft_rst_cmd)); + error = raydium_i2c_send(client, RM_RESET_MSG_ADDR, &soft_rst_cmd, + sizeof(soft_rst_cmd)); if (error) { dev_err(&client->dev, "software reset failed: %d\n", error); return error; @@ -295,9 +280,8 @@ static int raydium_i2c_query_ts_info(struct raydium_data *ts) if (error) continue; - error = raydium_i2c_read_message(client, - le32_to_cpu(query_bank_addr), - &ts->info, sizeof(ts->info)); + error = raydium_i2c_read(client, le32_to_cpu(query_bank_addr), + &ts->info, sizeof(ts->info)); if (error) continue; @@ -834,7 +818,7 @@ static irqreturn_t raydium_i2c_irq(int irq, void *_dev) if (ts->boot_mode != RAYDIUM_TS_MAIN) goto out; - error = raydium_i2c_read_message(ts->client, ts->data_bank_addr, + error = raydium_i2c_read(ts->client, ts->data_bank_addr, ts->report_data, ts->pkg_size); if (error) goto out; -- 2.28.0.297.g1956fa8f8d-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drivers: input: Use single i2c_transfer transaction when using RM_CMD_BANK_SWITCH 2020-08-21 2:40 ` [PATCH v2] " Furquan Shaikh @ 2020-09-09 0:44 ` Dmitry Torokhov 2020-09-14 6:04 ` Furquan Shaikh 0 siblings, 1 reply; 8+ messages in thread From: Dmitry Torokhov @ 2020-09-09 0:44 UTC (permalink / raw) To: Furquan Shaikh; +Cc: Dan Carpenter, linux-input, linux-kernel, adurbin 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; > } > > -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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drivers: input: Use single i2c_transfer transaction when using RM_CMD_BANK_SWITCH 2020-09-09 0:44 ` Dmitry Torokhov @ 2020-09-14 6:04 ` Furquan Shaikh 2020-09-14 6:34 ` Dmitry Torokhov 0 siblings, 1 reply; 8+ messages in thread From: Furquan Shaikh @ 2020-09-14 6:04 UTC (permalink / raw) To: Dmitry Torokhov Cc: Dan Carpenter, linux-input, Linux Kernel Mailing List, Aaron Durbin 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! - Furquan > > Thanks. > > -- > Dmitry ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drivers: input: Use single i2c_transfer transaction when using RM_CMD_BANK_SWITCH 2020-09-14 6:04 ` Furquan Shaikh @ 2020-09-14 6:34 ` Dmitry Torokhov 0 siblings, 0 replies; 8+ messages in thread From: Dmitry Torokhov @ 2020-09-14 6:34 UTC (permalink / raw) To: Furquan Shaikh Cc: Dan Carpenter, linux-input, Linux Kernel Mailing List, Aaron Durbin 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-09-14 6:34 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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).