Le mer. 25 mai 2022 à 15:42, Michael Zaidman <michael.zaidman@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx/, > > 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@xxxxxxxxx> > > > --- > > > 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. >
