| Message ID | 20201201060050.1193986-1-furquan@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | input: raydium_ts_i2c: Do not split tx transactions | expand |
Hi Furquan, On Mon, Nov 30, 2020 at 10:00:50PM -0800, Furquan Shaikh wrote: > Raydium device does not like splitting of tx transactions into > multiple messages - one for the register address and one for the > actual data. This results in incorrect behavior on the device side. > > This change updates raydium_i2c_read and raydium_i2c_write to create > i2c_msg arrays separately and passes those arrays into > raydium_i2c_xfer which decides based on the address whether the bank > switch command should be sent. The bank switch header is still added > by raydium_i2c_read and raydium_i2c_write to ensure that all these > operations are performed as part of a single I2C transfer. It > guarantees that no other transactions are initiated to any other > device on the same bus after the bank switch command is sent. i2c_transfer locks the bus [segment] for the entire time, so this explanation on why the change is needed does not make sense. Also, does it help if you mark the data message as I2C_M_NOSTART in case of writes? I also wonder if we should convert the driver to regmap, which should help with handling the bank switch as well as figuring out if it can do "gather write" or fall back to allocating an additional send buffer. Thanks.
Hello Dmitry, On Mon, Nov 30, 2020 at 10:28 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > Hi Furquan, > > On Mon, Nov 30, 2020 at 10:00:50PM -0800, Furquan Shaikh wrote: > > Raydium device does not like splitting of tx transactions into > > multiple messages - one for the register address and one for the > > actual data. This results in incorrect behavior on the device side. > > > > This change updates raydium_i2c_read and raydium_i2c_write to create > > i2c_msg arrays separately and passes those arrays into > > raydium_i2c_xfer which decides based on the address whether the bank > > switch command should be sent. The bank switch header is still added > > by raydium_i2c_read and raydium_i2c_write to ensure that all these > > operations are performed as part of a single I2C transfer. It > > guarantees that no other transactions are initiated to any other > > device on the same bus after the bank switch command is sent. > > i2c_transfer locks the bus [segment] for the entire time, so this > explanation on why the change is needed does not make sense. The actual problem is with raydium_i2c_write chopping off the write data into 2 messages -- one for register address and other for actual data. Raydium devices do not like that. Hence, this change to ensure that the register address and actual data are packaged into a single message. The latter part of the above comment attempts to explain why the bank switch message is added to xfer[] array in raydium_i2c_read and raydium_i2c_write instead of sending a separate message in raydium_i2c_xfer i.e. to ensure that the read/write xfer and bank switch are sent to i2c_transfer as a single array of messages so that they can be handled as an atomic operation from the perspective of communication with this device on the bus. > > Also, does it help if you mark the data message as I2C_M_NOSTART in case > of writes? That is a great suggestion. I think this would be helpful in this scenario. Let me follow-up on this to see if it helps with the current problem. > > I also wonder if we should convert the driver to regmap, which should > help with handling the bank switch as well as figuring out if it can do > "gather write" or fall back to allocating an additional send buffer. I will start with the above suggestion and fallback to this if that doesn't work. Thanks for the quick response and the helpful suggestions Dmitry. I will work on these pointers and get back to you. Thanks again. - Furquan > > Thanks. > > -- > Dmitry
On Mon, Nov 30, 2020 at 10:54:46PM -0800, Furquan Shaikh wrote: > Hello Dmitry, > > On Mon, Nov 30, 2020 at 10:28 PM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > > Hi Furquan, > > > > On Mon, Nov 30, 2020 at 10:00:50PM -0800, Furquan Shaikh wrote: > > > Raydium device does not like splitting of tx transactions into > > > multiple messages - one for the register address and one for the > > > actual data. This results in incorrect behavior on the device side. > > > > > > This change updates raydium_i2c_read and raydium_i2c_write to create > > > i2c_msg arrays separately and passes those arrays into > > > raydium_i2c_xfer which decides based on the address whether the bank > > > switch command should be sent. The bank switch header is still added > > > by raydium_i2c_read and raydium_i2c_write to ensure that all these > > > operations are performed as part of a single I2C transfer. It > > > guarantees that no other transactions are initiated to any other > > > device on the same bus after the bank switch command is sent. > > > > i2c_transfer locks the bus [segment] for the entire time, so this > > explanation on why the change is needed does not make sense. > > The actual problem is with raydium_i2c_write chopping off the write > data into 2 messages -- one for register address and other for actual > data. Raydium devices do not like that. Hence, this change to ensure > that the register address and actual data are packaged into a single > message. The latter part of the above comment attempts to explain why > the bank switch message is added to xfer[] array in raydium_i2c_read > and raydium_i2c_write instead of sending a separate message in > raydium_i2c_xfer i.e. to ensure that the read/write xfer and bank > switch are sent to i2c_transfer as a single array of messages so that > they can be handled as an atomic operation from the perspective of > communication with this device on the bus. OK, I see. > > > > > Also, does it help if you mark the data message as I2C_M_NOSTART in case > > of writes? > > That is a great suggestion. I think this would be helpful in this > scenario. Let me follow-up on this to see if it helps with the current > problem. > > > > > I also wonder if we should convert the driver to regmap, which should > > help with handling the bank switch as well as figuring out if it can do > > "gather write" or fall back to allocating an additional send buffer. > > I will start with the above suggestion and fallback to this if that > doesn't work. So my understanding is that not all I2C adapters support I2C_M_NOSTART so that is why regmap is nice as it hides it all away and figures things on its own. So simple solution of I2C_M_NOSTART might be a quick fix for Chrome OS kernel, but we'd either need to always use more expensive 2nd buffer as is in your patch, or regmap. Thanks.
On Mon, Nov 30, 2020 at 11:06 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > On Mon, Nov 30, 2020 at 10:54:46PM -0800, Furquan Shaikh wrote: > > Hello Dmitry, > > > > On Mon, Nov 30, 2020 at 10:28 PM Dmitry Torokhov > > <dmitry.torokhov@gmail.com> wrote: > > > > > > Hi Furquan, > > > > > > On Mon, Nov 30, 2020 at 10:00:50PM -0800, Furquan Shaikh wrote: > > > > Raydium device does not like splitting of tx transactions into > > > > multiple messages - one for the register address and one for the > > > > actual data. This results in incorrect behavior on the device side. > > > > > > > > This change updates raydium_i2c_read and raydium_i2c_write to create > > > > i2c_msg arrays separately and passes those arrays into > > > > raydium_i2c_xfer which decides based on the address whether the bank > > > > switch command should be sent. The bank switch header is still added > > > > by raydium_i2c_read and raydium_i2c_write to ensure that all these > > > > operations are performed as part of a single I2C transfer. It > > > > guarantees that no other transactions are initiated to any other > > > > device on the same bus after the bank switch command is sent. > > > > > > i2c_transfer locks the bus [segment] for the entire time, so this > > > explanation on why the change is needed does not make sense. > > > > The actual problem is with raydium_i2c_write chopping off the write > > data into 2 messages -- one for register address and other for actual > > data. Raydium devices do not like that. Hence, this change to ensure > > that the register address and actual data are packaged into a single > > message. The latter part of the above comment attempts to explain why > > the bank switch message is added to xfer[] array in raydium_i2c_read > > and raydium_i2c_write instead of sending a separate message in > > raydium_i2c_xfer i.e. to ensure that the read/write xfer and bank > > switch are sent to i2c_transfer as a single array of messages so that > > they can be handled as an atomic operation from the perspective of > > communication with this device on the bus. > > OK, I see. > > > > > > > > > Also, does it help if you mark the data message as I2C_M_NOSTART in case > > > of writes? > > > > That is a great suggestion. I think this would be helpful in this > > scenario. Let me follow-up on this to see if it helps with the current > > problem. > > > > > > > > I also wonder if we should convert the driver to regmap, which should > > > help with handling the bank switch as well as figuring out if it can do > > > "gather write" or fall back to allocating an additional send buffer. > > > > I will start with the above suggestion and fallback to this if that > > doesn't work. > > So my understanding is that not all I2C adapters support I2C_M_NOSTART > so that is why regmap is nice as it hides it all away and figures things > on its own. > > So simple solution of I2C_M_NOSTART might be a quick fix for Chrome OS > kernel, but we'd either need to always use more expensive 2nd buffer as > is in your patch, or regmap. Ah I see. That makes sense. In that case, I think switching to regmap would be better. As you suggested, I can use I2C_M_NOSTART as a quick fix and work on enabling regmap. > > Thanks. > > -- > Dmitry
diff --git a/drivers/input/touchscreen/raydium_i2c_ts.c b/drivers/input/touchscreen/raydium_i2c_ts.c index e694a9b2b1e5..259ecfdf33f1 100644 --- a/drivers/input/touchscreen/raydium_i2c_ts.c +++ b/drivers/input/touchscreen/raydium_i2c_ts.c @@ -137,45 +137,25 @@ struct raydium_data { bool wake_irq_enabled; }; -static int raydium_i2c_xfer(struct i2c_client *client, - u32 addr, void *data, size_t len, bool is_read) -{ - struct raydium_bank_switch_header { - u8 cmd; - __be32 be_addr; - } __packed header = { - .cmd = RM_CMD_BANK_SWITCH, - .be_addr = cpu_to_be32(addr), - }; - - u8 reg_addr = addr & 0xff; - - struct i2c_msg xfer[] = { - { - .addr = client->addr, - .len = sizeof(header), - .buf = (u8 *)&header, - }, - { - .addr = client->addr, - .len = 1, - .buf = ®_addr, - }, - { - .addr = client->addr, - .len = len, - .buf = data, - .flags = is_read ? I2C_M_RD : 0, - } - }; +/* + * Header to be sent for RM_CMD_BANK_SWITCH command. This is used by + * raydium_i2c_{read|send} below. + */ +struct __packed raydium_bank_switch_header { + u8 cmd; + __be32 be_addr; +}; +static int raydium_i2c_xfer(struct i2c_client *client, u32 addr, + struct i2c_msg *xfer, size_t xfer_count) +{ + int ret; /* * 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; + xfer_count -= xfer_start_idx; ret = i2c_transfer(client->adapter, &xfer[xfer_start_idx], xfer_count); if (likely(ret == xfer_count)) @@ -189,10 +169,43 @@ static int raydium_i2c_send(struct i2c_client *client, { int tries = 0; int error; + u8 *tx_buf; + u8 reg_addr = addr & 0xff; + + tx_buf = kmalloc(len + 1, GFP_KERNEL); + if (!tx_buf) + return -ENOMEM; + + tx_buf[0] = reg_addr; + memcpy(tx_buf + 1, data, len); do { - error = raydium_i2c_xfer(client, addr, (void *)data, len, - false); + struct raydium_bank_switch_header header = { + .cmd = RM_CMD_BANK_SWITCH, + .be_addr = cpu_to_be32(addr), + }; + + /* + * 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 + * issues. + */ + struct i2c_msg xfer[] = { + { + .addr = client->addr, + .len = sizeof(header), + .buf = (u8 *)&header, + }, + { + .addr = client->addr, + .len = len + 1, + .buf = tx_buf, + }, + }; + + error = raydium_i2c_xfer(client, addr, xfer, ARRAY_SIZE(xfer)); if (likely(!error)) return 0; @@ -206,12 +219,43 @@ static int raydium_i2c_send(struct i2c_client *client, static int raydium_i2c_read(struct i2c_client *client, u32 addr, void *data, size_t len) { - size_t xfer_len; int error; while (len) { - xfer_len = min_t(size_t, len, RM_MAX_READ_SIZE); - error = raydium_i2c_xfer(client, addr, data, xfer_len, true); + u8 reg_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 + * issues. + */ + struct i2c_msg xfer[] = { + { + .addr = client->addr, + .len = sizeof(header), + .buf = (u8 *)&header, + }, + { + .addr = client->addr, + .len = 1, + .buf = ®_addr, + }, + { + .addr = client->addr, + .len = xfer_len, + .buf = data, + .flags = I2C_M_RD, + } + }; + + error = raydium_i2c_xfer(client, addr, xfer, ARRAY_SIZE(xfer)); if (unlikely(error)) return error;
Raydium device does not like splitting of tx transactions into multiple messages - one for the register address and one for the actual data. This results in incorrect behavior on the device side. This change updates raydium_i2c_read and raydium_i2c_write to create i2c_msg arrays separately and passes those arrays into raydium_i2c_xfer which decides based on the address whether the bank switch command should be sent. The bank switch header is still added by raydium_i2c_read and raydium_i2c_write to ensure that all these operations are performed as part of a single I2C transfer. It guarantees that no other transactions are initiated to any other device on the same bus after the bank switch command is sent. Signed-off-by: Furquan Shaikh <furquan@google.com> --- drivers/input/touchscreen/raydium_i2c_ts.c | 120 ++++++++++++++------- 1 file changed, 82 insertions(+), 38 deletions(-)