Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 25 May 2022 11:44:21 +0300
From:   Michael Zaidman <michael.zaidman@gmail.com>
To:     Guillaume Champagne <champagne.guillaume.c@gmail.com>
Cc:     jikos@kernel.org, benjamin.tissoires@redhat.com,
        linux-i2c@vger.kernel.org, linux-input@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Mathieu Gallichand <mathieu.gallichand@sonatest.com>
Subject: Re: [PATCH] HID: ft260: fix multi packet i2c transactions
Message-ID: <20220525084421.GA7899@michael-VirtualBox>
References: <20220524192422.13967-1-champagne.guillaume.c@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220524192422.13967-1-champagne.guillaume.c@gmail.com>
Precedence: bulk

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