[PATCH] input: raydium_ts_i2c: Do not split tx transactions

[PATCH] input: raydium_ts_i2c: Do not split tx transactions

[Furquan Shaikh]

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(-)

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 = &reg_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 = &reg_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;
 
-- 
2.29.2.454.gaff20da3a2-goog

Re: [PATCH] input: raydium_ts_i2c: Do not split tx transactions

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH] input: raydium_ts_i2c: Do not split tx transactions

[Furquan Shaikh]

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

Re: [PATCH] input: raydium_ts_i2c: Do not split tx transactions

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH] input: raydium_ts_i2c: Do not split tx transactions

[Furquan Shaikh]

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

[PATCH v2] input: raydium_ts_i2c: Do not split tx transactions

[Furquan Shaikh]

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>
---
v2: Added comment in raydium_i2c_{send|read} about why regmap infrastructure
cannot be used for this driver as it splits bank switch and i2c read/write
into separate i2c_transfer calls which is known to create problems with
Raydium device.

 drivers/input/touchscreen/raydium_i2c_ts.c | 126 ++++++++++++++-------
 1 file changed, 88 insertions(+), 38 deletions(-)

diff --git a/drivers/input/touchscreen/raydium_i2c_ts.c b/drivers/input/touchscreen/raydium_i2c_ts.c
index e694a9b2b1e5..603a948460d6 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 = &reg_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,46 @@ 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. This is also why regmap infrastructure cannot be used
+		 * for this driver. Regmap handles page(bank) switch and reads
+		 * as separate i2c_transfer() operations. This can result in
+		 * problems if the Raydium device is on a shared I2C bus.
+		 */
+		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 +222,46 @@ 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. This is also why regmap infrastructure cannot be used
+		 * for this driver. Regmap handles page(bank) switch and writes
+		 * as separate i2c_transfer() operations. This can result in
+		 * problems if the Raydium device is on a shared I2C bus.
+		 */
+		struct i2c_msg xfer[] = {
+			{
+				.addr = client->addr,
+				.len = sizeof(header),
+				.buf = (u8 *)&header,
+			},
+			{
+				.addr = client->addr,
+				.len = 1,
+				.buf = &reg_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;
 
-- 
2.29.2.576.ga3fc446d84-goog

Re: [PATCH v2] input: raydium_ts_i2c: Do not split tx transactions

[Dmitry Torokhov]

On Fri, Dec 04, 2020 at 04:59:41PM -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.
> 
> Signed-off-by: Furquan Shaikh <furquan@google.com>

Applied, thank you.

-- 
Dmitry