* [PATCH v1 1/5] HID: ft260: ft260_xfer_status routine cleanup
2022-05-25 7:47 [PATCH v1 0/5] HID: ft260: fixes and performance improvements Michael Zaidman
@ 2022-05-25 7:47 ` Michael Zaidman
2022-05-25 7:47 ` [PATCH v1 2/5] HID: ft260: improve i2c write performance Michael Zaidman
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Michael Zaidman @ 2022-05-25 7:47 UTC (permalink / raw)
To: jikos, benjamin.tissoires, wsa
Cc: linux-kernel, linux-input, linux-i2c, champagne.guillaume.c,
mathieu.gallichand, Michael Zaidman
After clarifying with FTDI's support, it turned out that the error condition
(bit 1) in byte 1 of the i2c status HID report is a status bit reflecting all
error conditions. When bits 2, 3, or 4 are raised to 1, bit 1 is set to 1 also.
Since the ft260_xfer_status routine tests the error condition bit and exits
in the case of an error, the program flow never reaches the conditional
expressions for 2, 3, and 4 bits when any of them indicates an error state.
Though these expressions are never evaluated to true, they are checked several
times per IO, increasing the ft260_xfer_status polling cycle duration.
The patch removes the conditional expressions for 2, 3, and 4 bits in byte 1
of the i2c status HID report.
Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
---
drivers/hid/hid-ft260.c | 30 ++++++++++--------------------
1 file changed, 10 insertions(+), 20 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 79505c64dbfe..a35201d68b15 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -313,27 +313,17 @@ static int ft260_xfer_status(struct ft260_device *dev)
if (report.bus_status & FT260_I2C_STATUS_CTRL_BUSY)
return -EAGAIN;
- if (report.bus_status & FT260_I2C_STATUS_BUS_BUSY)
- return -EBUSY;
-
- if (report.bus_status & FT260_I2C_STATUS_ERROR)
+ /*
+ * The error condition (bit 1) is a status bit reflecting any
+ * error conditions. When any of the bits 2, 3, or 4 are raised
+ * to 1, bit 1 is also set to 1.
+ */
+ if (report.bus_status & FT260_I2C_STATUS_ERROR) {
+ hid_err(hdev, "i2c bus error: %#02x\n", report.bus_status);
return -EIO;
+ }
- ret = -EIO;
-
- if (report.bus_status & FT260_I2C_STATUS_ADDR_NO_ACK)
- ft260_dbg("unacknowledged address\n");
-
- if (report.bus_status & FT260_I2C_STATUS_DATA_NO_ACK)
- ft260_dbg("unacknowledged data\n");
-
- if (report.bus_status & FT260_I2C_STATUS_ARBITR_LOST)
- ft260_dbg("arbitration loss\n");
-
- if (report.bus_status & FT260_I2C_STATUS_CTRL_IDLE)
- ret = 0;
-
- return ret;
+ return 0;
}
static int ft260_hid_output_report(struct hid_device *hdev, u8 *data,
@@ -376,7 +366,7 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev,
break;
} while (--try);
- if (ret == 0 || ret == -EBUSY)
+ if (ret == 0)
return 0;
ft260_i2c_reset(hdev);
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v1 2/5] HID: ft260: improve i2c write performance
2022-05-25 7:47 [PATCH v1 0/5] HID: ft260: fixes and performance improvements Michael Zaidman
2022-05-25 7:47 ` [PATCH v1 1/5] HID: ft260: ft260_xfer_status routine cleanup Michael Zaidman
@ 2022-05-25 7:47 ` Michael Zaidman
2022-05-25 7:47 ` [PATCH v1 3/5] HID: ft260: support i2c writes larger than HID report size Michael Zaidman
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Michael Zaidman @ 2022-05-25 7:47 UTC (permalink / raw)
To: jikos, benjamin.tissoires, wsa
Cc: linux-kernel, linux-input, linux-i2c, champagne.guillaume.c,
mathieu.gallichand, Michael Zaidman
The patch improves i2c writing performance by about 30 percent by revising the
sleep time in the ft260_hid_output_report_check_status() in the following ways:
1. Reduce the sleep time and start to poll earlier:
Before:
$ sudo ./i2cperf -f 2 -o 2 -s 32 -r 0-0xff 13 0x51 -S
Fill block with increment via i2ctransfer by chunks
-------------------------------------------------------------------
data rate(bps) efficiency(%) data size(B) total IOs IO size(B)
-------------------------------------------------------------------
40510 80 256 8 32
After:
$ sudo ./i2cperf -f 2 -o 2 -s 32 -r 0-0xff 13 0x51 -S
Fill block with increment via i2ctransfer by chunks
-------------------------------------------------------------------
data rate(bps) efficiency(%) data size(B) total IOs IO size(B)
-------------------------------------------------------------------
52584 80 256 8 32
2. Do not sleep when the calculated sleep time is below 2 ms:
Before:
$ sudo ./i2cperf -f 2 -o 2 -s 16 -r 0-0xff 13 0x51 -S
Fill block with increment via i2ctransfer by chunks
-------------------------------------------------------------------
data rate(bps) efficiency(%) data size(B) total IOs IO size(B)
-------------------------------------------------------------------
26707 73 256 16 16
After:
$ sudo ./i2cperf -f 2 -o 2 -s 16 -r 0-0xff 13 0x51 -S
Fill block with increment via i2ctransfer by chunks
-------------------------------------------------------------------
data rate(bps) efficiency(%) data size(B) total IOs IO size(B)
-------------------------------------------------------------------
37034 73 256 16 16
Link to the i2cperf - https://github.com/MichaelZaidman/i2cperf
Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
---
drivers/hid/hid-ft260.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index a35201d68b15..44106cadd746 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -345,7 +345,7 @@ static int ft260_hid_output_report(struct hid_device *hdev, u8 *data,
static int ft260_hid_output_report_check_status(struct ft260_device *dev,
u8 *data, int len)
{
- int ret, usec, try = 3;
+ int ret, usec, try = 100;
struct hid_device *hdev = dev->hdev;
ret = ft260_hid_output_report(hdev, data, len);
@@ -356,10 +356,14 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev,
return ret;
}
- /* transfer time = 1 / clock(KHz) * 10 bits * bytes */
- usec = 10000 / dev->clock * len;
- usleep_range(usec, usec + 100);
- ft260_dbg("wait %d usec, len %d\n", usec, len);
+ /* transfer time = 1 / clock(KHz) * 9 bits * bytes */
+ usec = len * 9000 / dev->clock;
+ if (usec > 2000) {
+ usec -= 1500;
+ usleep_range(usec, usec + 100);
+ ft260_dbg("wait %d usec, len %d\n", usec, len);
+ }
+
do {
ret = ft260_xfer_status(dev);
if (ret != -EAGAIN)
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v1 3/5] HID: ft260: support i2c writes larger than HID report size
2022-05-25 7:47 [PATCH v1 0/5] HID: ft260: fixes and performance improvements Michael Zaidman
2022-05-25 7:47 ` [PATCH v1 1/5] HID: ft260: ft260_xfer_status routine cleanup Michael Zaidman
2022-05-25 7:47 ` [PATCH v1 2/5] HID: ft260: improve i2c write performance Michael Zaidman
@ 2022-05-25 7:47 ` Michael Zaidman
2022-05-25 15:44 ` Guillaume Champagne
2022-05-25 7:47 ` [PATCH v1 4/5] HID: ft260: support i2c reads greater " Michael Zaidman
2022-05-25 7:47 ` [PATCH v1 5/5] HID: ft260: improve i2c large reads performance Michael Zaidman
4 siblings, 1 reply; 9+ messages in thread
From: Michael Zaidman @ 2022-05-25 7:47 UTC (permalink / raw)
To: jikos, benjamin.tissoires, wsa
Cc: linux-kernel, linux-input, linux-i2c, champagne.guillaume.c,
mathieu.gallichand, Michael Zaidman
To support longer than one HID report size write, the driver splits a single
i2c message data payload into multiple i2c messages of HID report size.
However, it does not replicate the offset bytes within the EEPROM chip in
every consequent HID report because it is not and should not be aware of
the EEPROM type. It breaks the i2c write message integrity and causes the
EEPROM device not to acknowledge the second HID report keeping the i2c bus
busy until the ft260 controller reports failure.
This patch preserves the i2c write message integrity by manipulating the
i2c flag bits across multiple HID reports to be seen by the EEPROM device
as a single i2c write transfer.
Before:
$ sudo ./i2cperf -f 2 -o 2 -s 64 -r 0-0xff 13 0x51 -S
Error: Sending messages failed: Input/output error
[ +3.667741] ft260_i2c_write: rep 0xde addr 0x51 off 0 len 60 d[0] 0x0
[ +0.007330] ft260_hid_output_report_check_status: wait 6400 usec, len 64
[ +0.000203] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000001] ft260_i2c_write: rep 0xd1 addr 0x51 off 60 len 6 d[0] 0x0
[ +0.002337] ft260_hid_output_report_check_status: wait 1000 usec, len 10
[ +0.000157] ft260_xfer_status: bus_status 0x2e, clock 100
[ +0.000241] ft260_i2c_reset: done
[ +0.000003] ft260 0003:0403:6030.000E: ft260_i2c_write: failed to start transfer, ret -5
After:
$ sudo ./i2cperf -f 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S
Fill block with increment via i2ctransfer by chunks
-------------------------------------------------------------------
data rate(bps) efficiency(%) data size(B) total IOs IO size(B)
-------------------------------------------------------------------
58986 86 256 2 128
Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
---
drivers/hid/hid-ft260.c | 45 ++++++++++++++++++++++++-----------------
1 file changed, 27 insertions(+), 18 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 44106cadd746..bfda5b191a3a 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -378,41 +378,50 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev,
}
static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data,
- int data_len, u8 flag)
+ int len, u8 flag)
{
- int len, ret, idx = 0;
+ int ret, wr_len, idx = 0;
+ bool first = true;
struct hid_device *hdev = dev->hdev;
struct ft260_i2c_write_request_report *rep =
(struct ft260_i2c_write_request_report *)dev->write_buf;
do {
- if (data_len <= FT260_WR_DATA_MAX)
- len = data_len;
- else
- len = FT260_WR_DATA_MAX;
+ rep->flag = 0;
+ if (first) {
+ rep->flag = FT260_FLAG_START;
+ first = false;
+ }
+
+ if (len <= FT260_WR_DATA_MAX) {
+ wr_len = len;
+ if (flag == FT260_FLAG_START_STOP)
+ rep->flag |= FT260_FLAG_STOP;
+ } else {
+ wr_len = FT260_WR_DATA_MAX;
+ }
- rep->report = FT260_I2C_DATA_REPORT_ID(len);
+ rep->report = FT260_I2C_DATA_REPORT_ID(wr_len);
rep->address = addr;
- rep->length = len;
- rep->flag = flag;
+ rep->length = wr_len;
- memcpy(rep->data, &data[idx], len);
+ memcpy(rep->data, &data[idx], wr_len);
- ft260_dbg("rep %#02x addr %#02x off %d len %d d[0] %#02x\n",
- rep->report, addr, idx, len, data[0]);
+ ft260_dbg("rep %#02x addr %#02x off %d len %d wlen %d flag %#x d[0] %#02x\n",
+ rep->report, addr, idx, len, wr_len,
+ rep->flag, data[0]);
ret = ft260_hid_output_report_check_status(dev, (u8 *)rep,
- len + 4);
+ wr_len + 4);
if (ret < 0) {
- hid_err(hdev, "%s: failed to start transfer, ret %d\n",
- __func__, ret);
+ hid_err(hdev, "%s: failed with %d\n", __func__, ret);
return ret;
}
- data_len -= len;
- idx += len;
+ len -= wr_len;
+ idx += wr_len;
- } while (data_len > 0);
+ } while (len > 0);
return 0;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v1 3/5] HID: ft260: support i2c writes larger than HID report size
2022-05-25 7:47 ` [PATCH v1 3/5] HID: ft260: support i2c writes larger than HID report size Michael Zaidman
@ 2022-05-25 15:44 ` Guillaume Champagne
2022-05-25 19:42 ` Michael Zaidman
0 siblings, 1 reply; 9+ messages in thread
From: Guillaume Champagne @ 2022-05-25 15:44 UTC (permalink / raw)
To: Michael Zaidman
Cc: jikos, benjamin.tissoires, wsa, linux-kernel, linux-input,
linux-i2c, Mathieu Gallichand
Le mer. 25 mai 2022 à 03:48, Michael Zaidman
<michael.zaidman@gmail.com> a écrit :
>
> To support longer than one HID report size write, the driver splits a single
> i2c message data payload into multiple i2c messages of HID report size.
> However, it does not replicate the offset bytes within the EEPROM chip in
> every consequent HID report because it is not and should not be aware of
> the EEPROM type. It breaks the i2c write message integrity and causes the
> EEPROM device not to acknowledge the second HID report keeping the i2c bus
> busy until the ft260 controller reports failure.
>
I tested this whole patchset and it resolves the issue I raised
https://patchwork.kernel.org/project/linux-input/patch/20220524192422.13967-1-champagne.guillaume.c@gmail.com/,
thanks.
> This patch preserves the i2c write message integrity by manipulating the
> i2c flag bits across multiple HID reports to be seen by the EEPROM device
> as a single i2c write transfer.
>
> Before:
>
> $ sudo ./i2cperf -f 2 -o 2 -s 64 -r 0-0xff 13 0x51 -S
> Error: Sending messages failed: Input/output error
>
> [ +3.667741] ft260_i2c_write: rep 0xde addr 0x51 off 0 len 60 d[0] 0x0
> [ +0.007330] ft260_hid_output_report_check_status: wait 6400 usec, len 64
> [ +0.000203] ft260_xfer_status: bus_status 0x40, clock 100
> [ +0.000001] ft260_i2c_write: rep 0xd1 addr 0x51 off 60 len 6 d[0] 0x0
> [ +0.002337] ft260_hid_output_report_check_status: wait 1000 usec, len 10
> [ +0.000157] ft260_xfer_status: bus_status 0x2e, clock 100
> [ +0.000241] ft260_i2c_reset: done
> [ +0.000003] ft260 0003:0403:6030.000E: ft260_i2c_write: failed to start transfer, ret -5
>
> After:
>
> $ sudo ./i2cperf -f 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S
>
> Fill block with increment via i2ctransfer by chunks
> -------------------------------------------------------------------
> data rate(bps) efficiency(%) data size(B) total IOs IO size(B)
> -------------------------------------------------------------------
> 58986 86 256 2 128
>
> Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
> ---
> drivers/hid/hid-ft260.c | 45 ++++++++++++++++++++++++-----------------
> 1 file changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 44106cadd746..bfda5b191a3a 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
> @@ -378,41 +378,50 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev,
> }
>
> static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data,
> - int data_len, u8 flag)
> + int len, u8 flag)
> {
> - int len, ret, idx = 0;
> + int ret, wr_len, idx = 0;
> + bool first = true;
> struct hid_device *hdev = dev->hdev;
> struct ft260_i2c_write_request_report *rep =
> (struct ft260_i2c_write_request_report *)dev->write_buf;
>
> do {
> - if (data_len <= FT260_WR_DATA_MAX)
> - len = data_len;
> - else
> - len = FT260_WR_DATA_MAX;
> + rep->flag = 0;
> + if (first) {
> + rep->flag = FT260_FLAG_START;
I feel like multi packet transactions must still honor flag sent to
ft20_i2c_write. This adds a START even if ft260_i2c_write is called
with FT260_FLAG_START_REPEATED or FT260_FLAG_NONE.
> + first = false;
> + }
> +
> + if (len <= FT260_WR_DATA_MAX) {
> + wr_len = len;
> + if (flag == FT260_FLAG_START_STOP)
> + rep->flag |= FT260_FLAG_STOP;
> + } else {
> + wr_len = FT260_WR_DATA_MAX;
> + }
>
> - rep->report = FT260_I2C_DATA_REPORT_ID(len);
> + rep->report = FT260_I2C_DATA_REPORT_ID(wr_len);
> rep->address = addr;
> - rep->length = len;
> - rep->flag = flag;
> + rep->length = wr_len;
>
> - memcpy(rep->data, &data[idx], len);
> + memcpy(rep->data, &data[idx], wr_len);
>
> - ft260_dbg("rep %#02x addr %#02x off %d len %d d[0] %#02x\n",
> - rep->report, addr, idx, len, data[0]);
> + ft260_dbg("rep %#02x addr %#02x off %d len %d wlen %d flag %#x d[0] %#02x\n",
> + rep->report, addr, idx, len, wr_len,
> + rep->flag, data[0]);
>
> ret = ft260_hid_output_report_check_status(dev, (u8 *)rep,
> - len + 4);
> + wr_len + 4);
> if (ret < 0) {
> - hid_err(hdev, "%s: failed to start transfer, ret %d\n",
> - __func__, ret);
> + hid_err(hdev, "%s: failed with %d\n", __func__, ret);
> return ret;
> }
>
> - data_len -= len;
> - idx += len;
> + len -= wr_len;
> + idx += wr_len;
>
> - } while (data_len > 0);
> + } while (len > 0);
>
> return 0;
> }
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1 3/5] HID: ft260: support i2c writes larger than HID report size
2022-05-25 15:44 ` Guillaume Champagne
@ 2022-05-25 19:42 ` Michael Zaidman
2022-05-25 20:33 ` Guillaume Champagne
0 siblings, 1 reply; 9+ messages in thread
From: Michael Zaidman @ 2022-05-25 19:42 UTC (permalink / raw)
To: Guillaume Champagne
Cc: jikos, benjamin.tissoires, wsa, linux-kernel, linux-input,
linux-i2c, Mathieu Gallichand
On Wed, May 25, 2022 at 11:44:09AM -0400, Guillaume Champagne wrote:
> Le mer. 25 mai 2022 à 03:48, Michael Zaidman
> <michael.zaidman@gmail.com> a écrit :
> >
> > To support longer than one HID report size write, the driver splits a single
> > i2c message data payload into multiple i2c messages of HID report size.
> > However, it does not replicate the offset bytes within the EEPROM chip in
> > every consequent HID report because it is not and should not be aware of
> > the EEPROM type. It breaks the i2c write message integrity and causes the
> > EEPROM device not to acknowledge the second HID report keeping the i2c bus
> > busy until the ft260 controller reports failure.
> >
>
> I tested this whole patchset and it resolves the issue I raised
> https://patchwork.kernel.org/project/linux-input/patch/20220524192422.13967-1-champagne.guillaume.c@gmail.com/,
> thanks.
Much appreciated!
I will add your tested-by in the second version of the patchset.
>
> > This patch preserves the i2c write message integrity by manipulating the
> > i2c flag bits across multiple HID reports to be seen by the EEPROM device
> > as a single i2c write transfer.
> >
> > Before:
> >
> > $ sudo ./i2cperf -f 2 -o 2 -s 64 -r 0-0xff 13 0x51 -S
> > Error: Sending messages failed: Input/output error
> >
> > [ +3.667741] ft260_i2c_write: rep 0xde addr 0x51 off 0 len 60 d[0] 0x0
> > [ +0.007330] ft260_hid_output_report_check_status: wait 6400 usec, len 64
> > [ +0.000203] ft260_xfer_status: bus_status 0x40, clock 100
> > [ +0.000001] ft260_i2c_write: rep 0xd1 addr 0x51 off 60 len 6 d[0] 0x0
> > [ +0.002337] ft260_hid_output_report_check_status: wait 1000 usec, len 10
> > [ +0.000157] ft260_xfer_status: bus_status 0x2e, clock 100
> > [ +0.000241] ft260_i2c_reset: done
> > [ +0.000003] ft260 0003:0403:6030.000E: ft260_i2c_write: failed to start transfer, ret -5
> >
> > After:
> >
> > $ sudo ./i2cperf -f 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S
> >
> > Fill block with increment via i2ctransfer by chunks
> > -------------------------------------------------------------------
> > data rate(bps) efficiency(%) data size(B) total IOs IO size(B)
> > -------------------------------------------------------------------
> > 58986 86 256 2 128
> >
> > Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
> > ---
> > drivers/hid/hid-ft260.c | 45 ++++++++++++++++++++++++-----------------
> > 1 file changed, 27 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> > index 44106cadd746..bfda5b191a3a 100644
> > --- a/drivers/hid/hid-ft260.c
> > +++ b/drivers/hid/hid-ft260.c
> > @@ -378,41 +378,50 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev,
> > }
> >
> > static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data,
> > - int data_len, u8 flag)
> > + int len, u8 flag)
> > {
> > - int len, ret, idx = 0;
> > + int ret, wr_len, idx = 0;
> > + bool first = true;
> > struct hid_device *hdev = dev->hdev;
> > struct ft260_i2c_write_request_report *rep =
> > (struct ft260_i2c_write_request_report *)dev->write_buf;
> >
> > do {
> > - if (data_len <= FT260_WR_DATA_MAX)
> > - len = data_len;
> > - else
> > - len = FT260_WR_DATA_MAX;
> > + rep->flag = 0;
> > + if (first) {
> > + rep->flag = FT260_FLAG_START;
>
> I feel like multi packet transactions must still honor flag sent to
> ft20_i2c_write. This adds a START even if ft260_i2c_write is called
> with FT260_FLAG_START_REPEATED or FT260_FLAG_NONE.
We use the FT260_FLAG_START_REPEATED to precede the Read message following
the Write message in the i2c combined transaction. Am I missing any i2c
protocol case using the Repeated Start in the Write path?
The FT260_FLAG_NONE should not be passed into the ft20_i2c_write as well.
So, we can keep it simple.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1 3/5] HID: ft260: support i2c writes larger than HID report size
2022-05-25 19:42 ` Michael Zaidman
@ 2022-05-25 20:33 ` Guillaume Champagne
0 siblings, 0 replies; 9+ messages in thread
From: Guillaume Champagne @ 2022-05-25 20:33 UTC (permalink / raw)
To: Michael Zaidman
Cc: jikos, benjamin.tissoires, wsa, linux-kernel, linux-input,
linux-i2c, Mathieu Gallichand
Le mer. 25 mai 2022 à 15:42, Michael Zaidman
<michael.zaidman@gmail.com> a écrit :
>
> On Wed, May 25, 2022 at 11:44:09AM -0400, Guillaume Champagne wrote:
> > Le mer. 25 mai 2022 à 03:48, Michael Zaidman
> > <michael.zaidman@gmail.com> a écrit :
> > >
> > > To support longer than one HID report size write, the driver splits a single
> > > i2c message data payload into multiple i2c messages of HID report size.
> > > However, it does not replicate the offset bytes within the EEPROM chip in
> > > every consequent HID report because it is not and should not be aware of
> > > the EEPROM type. It breaks the i2c write message integrity and causes the
> > > EEPROM device not to acknowledge the second HID report keeping the i2c bus
> > > busy until the ft260 controller reports failure.
> > >
> >
> > I tested this whole patchset and it resolves the issue I raised
> > https://patchwork.kernel.org/project/linux-input/patch/20220524192422.13967-1-champagne.guillaume.c@gmail.com/,
> > thanks.
>
> Much appreciated!
> I will add your tested-by in the second version of the patchset.
>
> >
> > > This patch preserves the i2c write message integrity by manipulating the
> > > i2c flag bits across multiple HID reports to be seen by the EEPROM device
> > > as a single i2c write transfer.
> > >
> > > Before:
> > >
> > > $ sudo ./i2cperf -f 2 -o 2 -s 64 -r 0-0xff 13 0x51 -S
> > > Error: Sending messages failed: Input/output error
> > >
> > > [ +3.667741] ft260_i2c_write: rep 0xde addr 0x51 off 0 len 60 d[0] 0x0
> > > [ +0.007330] ft260_hid_output_report_check_status: wait 6400 usec, len 64
> > > [ +0.000203] ft260_xfer_status: bus_status 0x40, clock 100
> > > [ +0.000001] ft260_i2c_write: rep 0xd1 addr 0x51 off 60 len 6 d[0] 0x0
> > > [ +0.002337] ft260_hid_output_report_check_status: wait 1000 usec, len 10
> > > [ +0.000157] ft260_xfer_status: bus_status 0x2e, clock 100
> > > [ +0.000241] ft260_i2c_reset: done
> > > [ +0.000003] ft260 0003:0403:6030.000E: ft260_i2c_write: failed to start transfer, ret -5
> > >
> > > After:
> > >
> > > $ sudo ./i2cperf -f 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S
> > >
> > > Fill block with increment via i2ctransfer by chunks
> > > -------------------------------------------------------------------
> > > data rate(bps) efficiency(%) data size(B) total IOs IO size(B)
> > > -------------------------------------------------------------------
> > > 58986 86 256 2 128
> > >
> > > Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
> > > ---
> > > drivers/hid/hid-ft260.c | 45 ++++++++++++++++++++++++-----------------
> > > 1 file changed, 27 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> > > index 44106cadd746..bfda5b191a3a 100644
> > > --- a/drivers/hid/hid-ft260.c
> > > +++ b/drivers/hid/hid-ft260.c
> > > @@ -378,41 +378,50 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev,
> > > }
> > >
> > > static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data,
> > > - int data_len, u8 flag)
> > > + int len, u8 flag)
> > > {
> > > - int len, ret, idx = 0;
> > > + int ret, wr_len, idx = 0;
> > > + bool first = true;
> > > struct hid_device *hdev = dev->hdev;
> > > struct ft260_i2c_write_request_report *rep =
> > > (struct ft260_i2c_write_request_report *)dev->write_buf;
> > >
> > > do {
> > > - if (data_len <= FT260_WR_DATA_MAX)
> > > - len = data_len;
> > > - else
> > > - len = FT260_WR_DATA_MAX;
> > > + rep->flag = 0;
> > > + if (first) {
> > > + rep->flag = FT260_FLAG_START;
> >
> > I feel like multi packet transactions must still honor flag sent to
> > ft20_i2c_write. This adds a START even if ft260_i2c_write is called
> > with FT260_FLAG_START_REPEATED or FT260_FLAG_NONE.
>
> We use the FT260_FLAG_START_REPEATED to precede the Read message following
> the Write message in the i2c combined transaction. Am I missing any i2c
> protocol case using the Repeated Start in the Write path?
>
None that I know of. My point was that software wise it may be less
surprising to the programmer if "flag" is replicated as is when
calling ft260_i2c_write. For example, calling it with FT260_FLAG_STOP
only sends a START, no STOP. I agree that it isn't currently called
that way and that it may never be.
> The FT260_FLAG_NONE should not be passed into the ft20_i2c_write as well.
>
> So, we can keep it simple.
Agreed.
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v1 4/5] HID: ft260: support i2c reads greater than HID report size
2022-05-25 7:47 [PATCH v1 0/5] HID: ft260: fixes and performance improvements Michael Zaidman
` (2 preceding siblings ...)
2022-05-25 7:47 ` [PATCH v1 3/5] HID: ft260: support i2c writes larger than HID report size Michael Zaidman
@ 2022-05-25 7:47 ` Michael Zaidman
2022-05-25 7:47 ` [PATCH v1 5/5] HID: ft260: improve i2c large reads performance Michael Zaidman
4 siblings, 0 replies; 9+ messages in thread
From: Michael Zaidman @ 2022-05-25 7:47 UTC (permalink / raw)
To: jikos, benjamin.tissoires, wsa
Cc: linux-kernel, linux-input, linux-i2c, champagne.guillaume.c,
mathieu.gallichand, Michael Zaidman
A random i2c read operation in EEPROM devices is implemented as a dummy
write operation, followed by a current address read operation. The dummy
write operation is used to load the target byte or word address (a.k.a
offset) into the offset counter, from which the subsequent read operation
then reads.
To support longer than one HID report size random read, the ft260 driver
issues multiple pairs of i2c write offset + read data transactions of HID
report size so that the EEPROM device sees many i2c random read requests
from different offsets.
Two issues with the current implementation:
- This approach suffers from extra overhead caused by writing offset requests.
- Necessity to handle offset per HID report in big-endian representation as
EEPROM devices expect. The current implementation does not do it and
correctly handles the reads up to 60 bytes only.
This patch addresses both issues by implementing a more efficient approach.
It issues a single i2c read request of up to the EEPROM page size and then
waits for the data to arrive in multiple HID reports. For example, to read
the 256 bytes from a 24LC512 chip, which has 128 bytes page size, the old
method performs six ft260_i2c_write_read transactions while the new - two
only.
Before:
$ sudo ./i2cperf -d 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S
0x0000: 0x00 0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08 0x09 0x0a 0x0b 0x0c 0x0d 0x0e 0x0f
0x10 0x11 0x12 0x13 0x14 0x15 0x16 0x17 0x18 0x19 0x1a 0x1b 0x1c 0x1d 0x1e 0x1f
0x20 0x21 0x22 0x23 0x24 0x25 0x26 0x27 0x28 0x29 0x2a 0x2b 0x2c 0x2d 0x2e 0x2f
0x30 0x31 0x32 0x33 0x34 0x35 0x36 0x37 0x38 0x39 0x3a 0x3b 0x36 0x30 0x00 0x00
0x13 0x04 0xd5 0x00 0x1e 0x00 0x00 0x00 0xcd 0xa1 0x2f 0x00 0x6e 0x00 0x00 0x00
0xe1 0x5c 0xa4 0x5d 0x09 0x5d 0xdc 0x00 0xa6 0x01 0x0d 0x02 0x1c 0x00 0x0e 0x0c
0x04 0x01 0x00 0x27 0x36 0x30 0x00 0x00 0x9c 0x04 0xd5 0x00 0x1e 0x00 0x00 0x00
0xdf 0xa1 0x2f 0x00 0x6e 0x00 0x00 0x00 0x36 0x30 0x00 0x00 0xc2 0xe7 0xd5 0x00
0x0080: 0x80 0x81 0x82 0x83 0x84 0x85 0x86 0x87 0x88 0x89 0x8a 0x8b 0x8c 0x8d 0x8e 0x8f
0x90 0x91 0x92 0x93 0x94 0x95 0x96 0x97 0x98 0x99 0x9a 0x9b 0x9c 0x9d 0x9e 0x9f
0xa0 0xa1 0xa2 0xa3 0xa4 0xa5 0xa6 0xa7 0xa8 0xa9 0xaa 0xab 0xac 0xad 0xae 0xaf
0xb0 0xb1 0xb2 0xb3 0xb4 0xb5 0xb6 0xb7 0xb8 0xb9 0xba 0xbb 0x1e 0x00 0x00 0x00
0x06 0xa2 0x2f 0x00 0x6e 0x00 0x00 0x00 0x33 0x5d 0x29 0x5e 0xa0 0x5d 0xe1 0x00
0x9e 0x01 0x03 0x02 0x1c 0x00 0x0e 0x0c 0x04 0x02 0x1e 0x01 0x36 0x30 0x00 0x00
0x33 0x06 0xd5 0x00 0x1e 0x00 0x00 0x00 0x19 0xa2 0x2f 0x00 0x6e 0x00 0x00 0x00
0xa5 0x5d 0xfb 0x5d 0xe0 0x5d 0xdd 0x00 0x1e 0x00 0x00 0x00 0x69 0xc2 0x2f 0x00
Read block via i2ctransfer by chunks
-------------------------------------------------------------------
data rate(bps) efficiency(%) data size(B) total IOs IO size(B)
-------------------------------------------------------------------
40803 85 256 2 128
[ +2.376308] ft260_i2c_write_read: read_off 0x0 left_len 128 len 60
[ +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[ +0.000707] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000173] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 60
[ +0.008660] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.000156] ft260_xfer_status: bus_status 0x20, clock 100
[ +0.000001] ft260_i2c_write_read: read_off 0x3c left_len 68 len 60
[ +0.000001] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x3c
[ +0.001034] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000191] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 60
[ +0.008614] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.000203] ft260_xfer_status: bus_status 0x20, clock 100
[ +0.000001] ft260_i2c_write_read: read_off 0x78 left_len 8 len 8
[ +0.000001] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x78
[ +0.000987] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000192] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 8
[ +0.002614] ft260_raw_event: i2c resp: rep 0xd1 len 8
[ +0.000200] ft260_xfer_status: bus_status 0x20, clock 100
[ +0.012511] ft260_i2c_write_read: read_off 0x8000 left_len 128 len 60
[ +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[ +0.001456] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000207] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 60
[ +0.008625] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.000203] ft260_xfer_status: bus_status 0x20, clock 100
[ +0.000002] ft260_i2c_write_read: read_off 0x803c left_len 68 len 60
[ +0.000001] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x3c
[ +0.000991] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000196] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 60
[ +0.008621] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.000202] ft260_xfer_status: bus_status 0x20, clock 100
[ +0.000001] ft260_i2c_write_read: read_off 0x8078 left_len 8 len 8
[ +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x78
[ +0.000979] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000151] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 8
[ +0.002644] ft260_raw_event: i2c resp: rep 0xd1 len 8
[ +0.000141] ft260_xfer_status: bus_status 0x20, clock 100
After:
$ sudo ./i2cperf -d 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S
0x0000: 0x00 0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08 0x09 0x0a 0x0b 0x0c 0x0d 0x0e 0x0f
0x10 0x11 0x12 0x13 0x14 0x15 0x16 0x17 0x18 0x19 0x1a 0x1b 0x1c 0x1d 0x1e 0x1f
0x20 0x21 0x22 0x23 0x24 0x25 0x26 0x27 0x28 0x29 0x2a 0x2b 0x2c 0x2d 0x2e 0x2f
0x30 0x31 0x32 0x33 0x34 0x35 0x36 0x37 0x38 0x39 0x3a 0x3b 0x3c 0x3d 0x3e 0x3f
0x40 0x41 0x42 0x43 0x44 0x45 0x46 0x47 0x48 0x49 0x4a 0x4b 0x4c 0x4d 0x4e 0x4f
0x50 0x51 0x52 0x53 0x54 0x55 0x56 0x57 0x58 0x59 0x5a 0x5b 0x5c 0x5d 0x5e 0x5f
0x60 0x61 0x62 0x63 0x64 0x65 0x66 0x67 0x68 0x69 0x6a 0x6b 0x6c 0x6d 0x6e 0x6f
0x70 0x71 0x72 0x73 0x74 0x75 0x76 0x77 0x78 0x79 0x7a 0x7b 0x7c 0x7d 0x7e 0x7f
0x0080: 0x80 0x81 0x82 0x83 0x84 0x85 0x86 0x87 0x88 0x89 0x8a 0x8b 0x8c 0x8d 0x8e 0x8f
0x90 0x91 0x92 0x93 0x94 0x95 0x96 0x97 0x98 0x99 0x9a 0x9b 0x9c 0x9d 0x9e 0x9f
0xa0 0xa1 0xa2 0xa3 0xa4 0xa5 0xa6 0xa7 0xa8 0xa9 0xaa 0xab 0xac 0xad 0xae 0xaf
0xb0 0xb1 0xb2 0xb3 0xb4 0xb5 0xb6 0xb7 0xb8 0xb9 0xba 0xbb 0xbc 0xbd 0xbe 0xbf
0xc0 0xc1 0xc2 0xc3 0xc4 0xc5 0xc6 0xc7 0xc8 0xc9 0xca 0xcb 0xcc 0xcd 0xce 0xcf
0xd0 0xd1 0xd2 0xd3 0xd4 0xd5 0xd6 0xd7 0xd8 0xd9 0xda 0xdb 0xdc 0xdd 0xde 0xdf
0xe0 0xe1 0xe2 0xe3 0xe4 0xe5 0xe6 0xe7 0xe8 0xe9 0xea 0xeb 0xec 0xed 0xee 0xef
0xf0 0xf1 0xf2 0xf3 0xf4 0xf5 0xf6 0xf7 0xf8 0xf9 0xfa 0xfb 0xfc 0xfd 0xfe 0xff
Read block via i2ctransfer by chunks
-------------------------------------------------------------------
data rate(bps) efficiency(%) data size(B) total IOs IO size(B)
-------------------------------------------------------------------
43990 85 256 2 128
[ +1.464346] ft260_i2c_write_read: off 0x0 rlen 128 wlen 2
[ +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[ +0.001653] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000188] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 128 rlen 60 flag 0x3
[ +0.008609] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.000157] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 68 rlen 60 flag 0x0
[ +0.008840] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.000203] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 8 rlen 8 flag 0x4
[ +0.002794] ft260_raw_event: i2c resp: rep 0xd1 len 8
[ +0.000201] ft260_xfer_status: bus_status 0x20, clock 100
[ +0.015171] ft260_i2c_write_read: off 0x80 rlen 128 wlen 2
[ +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[ +0.000764] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000231] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000148] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 128 rlen 60 flag 0x3
[ +0.008488] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.000205] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 68 rlen 60 flag 0x0
[ +0.008795] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.000176] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 8 rlen 8 flag 0x4
[ +0.002819] ft260_raw_event: i2c resp: rep 0xd1 len 8
[ +0.000167] ft260_xfer_status: bus_status 0x20, clock 100
Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
---
drivers/hid/hid-ft260.c | 133 ++++++++++++++++++++++------------------
1 file changed, 72 insertions(+), 61 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index bfda5b191a3a..cb8f1782d1f0 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -460,49 +460,68 @@ static int ft260_smbus_write(struct ft260_device *dev, u8 addr, u8 cmd,
static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
u16 len, u8 flag)
{
+ u16 rd_len;
+ int timeout, ret;
struct ft260_i2c_read_request_report rep;
struct hid_device *hdev = dev->hdev;
- int timeout;
- int ret;
+ bool first = true;
- if (len > FT260_RD_DATA_MAX) {
- hid_err(hdev, "%s: unsupported rd len: %d\n", __func__, len);
- return -EINVAL;
- }
+ do {
+ if (first) {
+ if (flag & FT260_FLAG_START_REPEATED)
+ flag = FT260_FLAG_START_REPEATED;
+ else
+ flag = FT260_FLAG_START;
+ first = false;
+ } else {
+ flag = 0;
+ }
- dev->read_idx = 0;
- dev->read_buf = data;
- dev->read_len = len;
+ if (len <= FT260_RD_DATA_MAX) {
+ rd_len = len;
+ flag |= FT260_FLAG_STOP;
+ } else {
+ rd_len = FT260_RD_DATA_MAX;
+ }
- rep.report = FT260_I2C_READ_REQ;
- rep.length = cpu_to_le16(len);
- rep.address = addr;
- rep.flag = flag;
+ dev->read_idx = 0;
+ dev->read_buf = data;
+ dev->read_len = rd_len;
- ft260_dbg("rep %#02x addr %#02x len %d\n", rep.report, rep.address,
- rep.length);
+ rep.report = FT260_I2C_READ_REQ;
+ rep.length = cpu_to_le16(rd_len);
+ rep.address = addr;
+ rep.flag = flag;
- reinit_completion(&dev->wait);
+ ft260_dbg("rep %#02x addr %#02x len %d rlen %d flag %#x\n",
+ rep.report, rep.address, len, rd_len, flag);
- ret = ft260_hid_output_report(hdev, (u8 *)&rep, sizeof(rep));
- if (ret < 0) {
- hid_err(hdev, "%s: failed to start transaction, ret %d\n",
- __func__, ret);
- return ret;
- }
+ reinit_completion(&dev->wait);
- timeout = msecs_to_jiffies(5000);
- if (!wait_for_completion_timeout(&dev->wait, timeout)) {
- ft260_i2c_reset(hdev);
- return -ETIMEDOUT;
- }
+ ret = ft260_hid_output_report(hdev, (u8 *)&rep, sizeof(rep));
+ if (ret < 0) {
+ hid_err(hdev, "%s: failed with %d\n", __func__, ret);
+ return ret;
+ }
- ret = ft260_xfer_status(dev);
- if (ret == 0)
- return 0;
+ timeout = msecs_to_jiffies(5000);
+ if (!wait_for_completion_timeout(&dev->wait, timeout)) {
+ ft260_i2c_reset(hdev);
+ return -ETIMEDOUT;
+ }
- ft260_i2c_reset(hdev);
- return -EIO;
+ ret = ft260_xfer_status(dev);
+ if (ret < 0) {
+ ft260_i2c_reset(hdev);
+ return -EIO;
+ }
+
+ len -= rd_len;
+ data += rd_len;
+
+ } while (len > 0);
+
+ return 0;
}
/*
@@ -513,45 +532,37 @@ static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
*/
static int ft260_i2c_write_read(struct ft260_device *dev, struct i2c_msg *msgs)
{
- int len, ret;
- u16 left_len = msgs[1].len;
- u8 *read_buf = msgs[1].buf;
+ int ret;
+ int wr_len = msgs[0].len;
+ int rd_len = msgs[1].len;
+ struct hid_device *hdev = dev->hdev;
u8 addr = msgs[0].addr;
u16 read_off = 0;
- struct hid_device *hdev = dev->hdev;
- if (msgs[0].len > 2) {
- hid_err(hdev, "%s: unsupported wr len: %d\n", __func__,
- msgs[0].len);
+ if (wr_len > 2) {
+ hid_err(hdev, "%s: invalid wr_len: %d\n", __func__, wr_len);
return -EOPNOTSUPP;
}
- memcpy(&read_off, msgs[0].buf, msgs[0].len);
-
- do {
- if (left_len <= FT260_RD_DATA_MAX)
- len = left_len;
+ if (ft260_debug) {
+ if (wr_len == 2)
+ read_off = be16_to_cpu(*(u16 *)msgs[0].buf);
else
- len = FT260_RD_DATA_MAX;
-
- ft260_dbg("read_off %#x left_len %d len %d\n", read_off,
- left_len, len);
-
- ret = ft260_i2c_write(dev, addr, (u8 *)&read_off, msgs[0].len,
- FT260_FLAG_START);
- if (ret < 0)
- return ret;
+ read_off = *msgs[0].buf;
- ret = ft260_i2c_read(dev, addr, read_buf, len,
- FT260_FLAG_START_STOP);
- if (ret < 0)
- return ret;
+ pr_info("%s: off %#x rlen %d wlen %d\n", __func__,
+ read_off, rd_len, wr_len);
+ }
- left_len -= len;
- read_buf += len;
- read_off += len;
+ ret = ft260_i2c_write(dev, addr, msgs[0].buf, wr_len,
+ FT260_FLAG_START);
+ if (ret < 0)
+ return ret;
- } while (left_len > 0);
+ ret = ft260_i2c_read(dev, addr, msgs[1].buf, rd_len,
+ FT260_FLAG_START_STOP_REPEATED);
+ if (ret < 0)
+ return ret;
return 0;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v1 5/5] HID: ft260: improve i2c large reads performance
2022-05-25 7:47 [PATCH v1 0/5] HID: ft260: fixes and performance improvements Michael Zaidman
` (3 preceding siblings ...)
2022-05-25 7:47 ` [PATCH v1 4/5] HID: ft260: support i2c reads greater " Michael Zaidman
@ 2022-05-25 7:47 ` Michael Zaidman
4 siblings, 0 replies; 9+ messages in thread
From: Michael Zaidman @ 2022-05-25 7:47 UTC (permalink / raw)
To: jikos, benjamin.tissoires, wsa
Cc: linux-kernel, linux-input, linux-i2c, champagne.guillaume.c,
mathieu.gallichand, Michael Zaidman
The patch increases the read buffer size to 128 bytes. It reduces the number
of ft260_i2c_read calls by three and improves the large data chunks' read
performance by about 10%.
Before:
$ sudo ./i2cperf -d 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S
Read block via i2ctransfer by chunks
-------------------------------------------------------------------
data rate(bps) efficiency(%) data size(B) total IOs IO size(B)
-------------------------------------------------------------------
43990 85 256 2 128
[ +1.464346] ft260_i2c_write_read: off 0x0 rlen 128 wlen 2
[ +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[ +0.001653] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000188] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 128 rlen 60 flag 0x3
[ +0.008609] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.000157] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 68 rlen 60 flag 0x0
[ +0.008840] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.000203] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 8 rlen 8 flag 0x4
[ +0.002794] ft260_raw_event: i2c resp: rep 0xd1 len 8
[ +0.000201] ft260_xfer_status: bus_status 0x20, clock 100
[ +0.015171] ft260_i2c_write_read: off 0x80 rlen 128 wlen 2
[ +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[ +0.000764] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000231] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000148] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 128 rlen 60 flag 0x3
[ +0.008488] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.000205] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000002] ft260_i2c_read: rep 0xc2 addr 0x51 len 68 rlen 60 flag 0x0
[ +0.008795] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.000176] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 8 rlen 8 flag 0x4
[ +0.002819] ft260_raw_event: i2c resp: rep 0xd1 len 8
[ +0.000167] ft260_xfer_status: bus_status 0x20, clock 100
After:
$ sudo ./i2cperf -d 2 -o 2 -s 128 -r 0-0xff 13 0x51 -S
Read block via i2ctransfer by chunks
-------------------------------------------------------------------
data rate(bps) efficiency(%) data size(B) total IOs IO size(B)
-------------------------------------------------------------------
49316 85 256 2 128
[ +1.447360] ft260_i2c_write_read: off 0x0 rlen 128 wlen 2
[ +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[ +0.001633] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000190] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 128 rlen 128 flag 0x7
[ +0.008617] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.008033] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.000954] ft260_raw_event: i2c resp: rep 0xd1 len 8
[ +0.000208] ft260_xfer_status: bus_status 0x20, clock 100
[ +0.015853] ft260_i2c_write_read: off 0x80 rlen 128 wlen 2
[ +0.000002] ft260_i2c_write: rep 0xd0 addr 0x51 off 0 len 2 wlen 2 flag 0x2 d[0] 0x0
[ +0.001182] ft260_xfer_status: bus_status 0x41, clock 100
[ +0.000180] ft260_xfer_status: bus_status 0x40, clock 100
[ +0.000001] ft260_i2c_read: rep 0xc2 addr 0x51 len 128 rlen 128 flag 0x7
[ +0.008575] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.008014] ft260_raw_event: i2c resp: rep 0xde len 60
[ +0.001001] ft260_raw_event: i2c resp: rep 0xd1 len 8
[ +0.000223] ft260_xfer_status: bus_status 0x20, clock 100
Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
---
drivers/hid/hid-ft260.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index cb8f1782d1f0..c7c3a9d395a2 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -30,12 +30,8 @@ MODULE_PARM_DESC(debug, "Toggle FT260 debugging messages");
#define FT260_REPORT_MAX_LENGTH (64)
#define FT260_I2C_DATA_REPORT_ID(len) (FT260_I2C_REPORT_MIN + (len - 1) / 4)
-/*
- * The input report format assigns 62 bytes for the data payload, but ft260
- * returns 60 and 2 in two separate transactions. To minimize transfer time
- * in reading chunks mode, set the maximum read payload length to 60 bytes.
- */
-#define FT260_RD_DATA_MAX (60)
+
+#define FT260_RD_DATA_MAX (128)
#define FT260_WR_DATA_MAX (60)
/*
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread