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>
In-Reply-To: <20220525074757.7519-4-michael.zaidman@gmail.com>
From:   Guillaume Champagne <champagne.guillaume.c@gmail.com>
Date:   Wed, 25 May 2022 11:44:09 -0400
Message-ID: <CAHSN6OfdHfiZRXjnqvnpcbwS0nt0vBuLiwMHcbpsd_AWiJ3i1g@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 03:48, Michael Zaidman
<michael.zaidman@gmail.com> a =C3=A9crit :
>
> To support longer than one HID report size write, the driver splits a sin=
gle
> 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 bu=
s
> 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 6=
4
> [  +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 1=
0
> [  +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(str=
uct 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 =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_buf;
>
>         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.

> +                       first =3D false;
> +               }
> +
> +               if (len <=3D FT260_WR_DATA_MAX) {
> +                       wr_len =3D len;
> +                       if (flag =3D=3D FT260_FLAG_START_STOP)
> +                               rep->flag |=3D FT260_FLAG_STOP;
> +               } else {
> +                       wr_len =3D FT260_WR_DATA_MAX;
> +               }
>
> -               rep->report =3D FT260_I2C_DATA_REPORT_ID(len);
> +               rep->report =3D FT260_I2C_DATA_REPORT_ID(wr_len);
>                 rep->address =3D addr;
> -               rep->length =3D len;
> -               rep->flag =3D flag;
> +               rep->length =3D 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 fla=
g %#x d[0] %#02x\n",
> +                         rep->report, addr, idx, len, wr_len,
> +                         rep->flag, data[0]);
>
>                 ret =3D ft260_hid_output_report_check_status(dev, (u8 *)r=
ep,
> -                                                          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__, r=
et);
>                         return ret;
>                 }
>
> -               data_len -=3D len;
> -               idx +=3D len;
> +               len -=3D wr_len;
> +               idx +=3D wr_len;
>
> -       } while (data_len > 0);
> +       } while (len > 0);
>
>         return 0;
>  }
> --
> 2.25.1
>