[PATCH] HID: ft260: fix multi packet i2c transactions

[PATCH] HID: ft260: fix multi packet i2c transactions

[Guillaume Champagne]

Only trigger START and STOP conditions for the first and last HID
packets when i2c writes are split in multiple packets. Otherwise, slave
i2c devices receive each packet as standalone i2c transactions. Since
i2c slave devices clear their internal state on STOP, this breaks auto
increment of the register address written to.

Concretely, SCL is now held low between processing of HID packets so i2c
slave devices know to keep increment the same register address when the
next bytes arrive.

Co-developed-by: Mathieu Gallichand <mathieu.gallichand@sonatest.com>
Signed-off-by: Mathieu Gallichand <mathieu.gallichand@sonatest.com>
Signed-off-by: Guillaume Champagne <champagne.guillaume.c@gmail.com>
---
 drivers/hid/hid-ft260.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 79505c64dbfe..9c5912a21ccb 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -390,6 +390,8 @@ static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data,
 	struct hid_device *hdev = dev->hdev;
 	struct ft260_i2c_write_request_report *rep =
 		(struct ft260_i2c_write_request_report *)dev->write_buf;
+	bool multi_packet = data_len > FT260_WR_DATA_MAX;
+	u8 packet_flag = multi_packet ? flag & FT260_FLAG_START_REPEATED : flag;
 
 	do {
 		if (data_len <= FT260_WR_DATA_MAX)
@@ -400,7 +402,7 @@ static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data,
 		rep->report = FT260_I2C_DATA_REPORT_ID(len);
 		rep->address = addr;
 		rep->length = len;
-		rep->flag = flag;
+		rep->flag = packet_flag;
 
 		memcpy(rep->data, &data[idx], len);
 
@@ -418,6 +420,12 @@ static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data,
 		data_len -= len;
 		idx += len;
 
+		if (multi_packet) {
+			if (data_len <= FT260_WR_DATA_MAX)
+				packet_flag = flag & FT260_FLAG_STOP;
+			else
+				packet_flag = FT260_FLAG_NONE;
+		}
 	} while (data_len > 0);
 
 	return 0;
-- 
2.20.1

Re: [PATCH] HID: ft260: fix multi packet i2c transactions

[Michael Zaidman]

On Tue, May 24, 2022 at 03:24:22PM -0400, Guillaume Champagne wrote:
> Only trigger START and STOP conditions for the first and last HID
> packets when i2c writes are split in multiple packets. Otherwise, slave
> i2c devices receive each packet as standalone i2c transactions. Since
> i2c slave devices clear their internal state on STOP, this breaks auto
> increment of the register address written to.
> 
> Concretely, SCL is now held low between processing of HID packets so i2c
> slave devices know to keep increment the same register address when the
> next bytes arrive.
> 
> Co-developed-by: Mathieu Gallichand <mathieu.gallichand@sonatest.com>
> Signed-off-by: Mathieu Gallichand <mathieu.gallichand@sonatest.com>
> Signed-off-by: Guillaume Champagne <champagne.guillaume.c@gmail.com>
> ---
>  drivers/hid/hid-ft260.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 79505c64dbfe..9c5912a21ccb 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
> @@ -390,6 +390,8 @@ static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data,
>  	struct hid_device *hdev = dev->hdev;
>  	struct ft260_i2c_write_request_report *rep =
>  		(struct ft260_i2c_write_request_report *)dev->write_buf;
> +	bool multi_packet = data_len > FT260_WR_DATA_MAX;
> +	u8 packet_flag = multi_packet ? flag & FT260_FLAG_START_REPEATED : flag;

Please take a look at the https://patchwork.kernel.org/project/linux-input/patch/20220525074757.7519-4-michael.zaidman@gmail.com/
that addresses the same issue but does it more efficiently by adding one
conditional statement per ft260_i2c_write call in the main path vs. three
in this commit. It comes in the patch set with other performance improvements
published on my GitHub https://github.com/MichaelZaidman/hid-ft260 several
months ago. I would greatly appreciate it if you could test and feedback
on the patch set content.

Thanks,
Michael