Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220525074757.7519-1-michael.zaidman@gmail.com>
 <20220525074757.7519-4-michael.zaidman@gmail.com> <CAHSN6OfdHfiZRXjnqvnpcbwS0nt0vBuLiwMHcbpsd_AWiJ3i1g@mail.gmail.com>
 <20220525194242.GA13454@michael-VirtualBox>
In-Reply-To: <20220525194242.GA13454@michael-VirtualBox>
From:   Guillaume Champagne <champagne.guillaume.c@gmail.com>
Date:   Wed, 25 May 2022 16:33:09 -0400
Message-ID: <CAHSN6Ocr538DZCkP9-RL7V7ALc6G_e4rWQz56qRg4o-GVd7=hg@mail.gmail.com>
Subject: Re: [PATCH v1 3/5] HID: ft260: support i2c writes larger than HID
 report size
To:     Michael Zaidman <michael.zaidman@gmail.com>
Cc:     jikos@kernel.org, benjamin.tissoires@redhat.com, wsa@kernel.org,
        linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
        linux-i2c@vger.kernel.org,
        Mathieu Gallichand <mathieu.gallichand@sonatest.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Le mer. 25 mai 2022 =C3=A0 15:42, Michael Zaidman
<michael.zaidman@gmail.com> a =C3=A9crit :
>
> On Wed, May 25, 2022 at 11:44:09AM -0400, Guillaume Champagne wrote:
> > Le mer. 25 mai 2022 =C3=A0 03:48, Michael Zaidman
> > <michael.zaidman@gmail.com> a =C3=A9crit :
> > >
> > > 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 siz=
e.
> > > However, it does not replicate the offset bytes within the EEPROM chi=
p 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 i2=
c 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.1=
3967-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 de=
vice
> > > 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] 0=
x0
> > > [  +0.007330] ft260_hid_output_report_check_status: wait 6400 usec, l=
en 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] 0=
x0
> > > [  +0.002337] ft260_hid_output_report_check_status: wait 1000 usec, l=
en 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 s=
tart 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 *da=
ta,
> > > -                          int data_len, u8 flag)
> > > +                          int len, u8 flag)
> > >  {
> > > -       int len, ret, idx =3D 0;
> > > +       int ret, wr_len, idx =3D 0;
> > > +       bool first =3D true;
> > >         struct hid_device *hdev =3D dev->hdev;
> > >         struct ft260_i2c_write_request_report *rep =3D
> > >                 (struct ft260_i2c_write_request_report *)dev->write_b=
uf;
> > >
> > >         do {
> > > -               if (data_len <=3D FT260_WR_DATA_MAX)
> > > -                       len =3D data_len;
> > > -               else
> > > -                       len =3D FT260_WR_DATA_MAX;
> > > +               rep->flag =3D 0;
> > > +               if (first) {
> > > +                       rep->flag =3D 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 followin=
g
> 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.

>