Subject: Re: [PATCH v4 0/4] Add support for the Cypress cyttsp5
To:     Alistair Francis <alistair@alistair23.me>,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc:     dmitry.torokhov@gmail.com, alistair23@gmail.com,
        robh+dt@kernel.org, linus.walleij@linaro.org, rydberg@bitmath.org,
        andreas@kemnade.info
References: <20211222124603.326920-1-alistair@alistair23.me>
From:   Samuel Holland <samuel@sholland.org>
Message-ID: <37291fe7-7028-cd9e-7fde-01d13f85f0d9@sholland.org>
Date:   Thu, 6 Jan 2022 23:49:50 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <20211222124603.326920-1-alistair@alistair23.me>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hello!

On 12/22/21 6:45 AM, Alistair Francis wrote:
> This patch series builds on top of [1] and adds support for the cyttsp5
> touchscreen controller for the reMarkable 2.
> 
> I first tried to add an I2C HID device. Although the cyttsp5 has some HID
> looking aspects it is not HID compatible. Just in trying to probe the device
> I found:
>  - The HID descriptor has extra padding
>  - The HID descriptor sets the high bytes of the descriptor length
>  - The HID descriptor has extra unrecognised tags
>  - The HID reset command doesn't appear to work
> 
> I don't think there is a way to use the I2C HID framework with the cyttsp5.
> For anyone interested you can see the work here [2]. In that branch though I
> can only obtain a HID descriptor, nothing else works without more core
> changes.
> 
> So instead I rebased the series from [1]. Converted to the new yaml DTS
> documentation, added regulator support and fixed a x/y miscalculation bug.

I am working on the PineNote, which also uses a cyttsp5 touchscreen. I attempted
to use the driver from this series, but I could not get it to work. I found that
the cyttsp5_sensing_conf_data was filled with all zeroes, so every touch failed
the max_tch check. I noticed that cmd_done was being completed by empty
responses (which explains why the response buffer was zeroes), but I got stuck
after that.

So I looked back at the thread you linked below, and tried to implement the
workarounds described there, and those above, plus some others, and I was able
to get the touchscreen working with i2c_hid. Here are the changes I made to i2c_hid:
https://github.com/smaeul/linux/commit/a1e07425a6c4

In summary:
 - Perform a 2-byte dummy read before reading the HID descriptor.
   This is required to clear the two-byte empty message.
 - Split command/response into multiple I2C transactions.
   This is probably some sort of timing issue.
   Without these first two, HID descriptor reads return "02 00".
 - Chop 2 bytes out of the HID descriptor, as per the thread below.
 - Similarly, chop 3 bytes out of the report descriptor.
 - Skip the reset command, as above. Otherwise, the touchscreen
   sends a different, short, unusable (partial?) report descriptor.

I reused the the existing i2c_hid_of_goodix driver to handle toggling the reset
line, which is required. That existing binding is almost identical to the one in
this series. Here's the glue I added:
https://github.com/smaeul/linux/commit/65d9250d3899

And here is the result, from dmesg and debugfs:
https://gist.github.com/smaeul/60b4b0f784bfff8bb8ce3ee3b4483be9

So far, the quirks only appear to affect probing the device. The touchscreen
works normally after that.

What do you think of this approach? It certainly seems cleaner than parsing the
HID reports/responses by hand. But I don't know if all of the quirks are
acceptable for i2c_hid.

One additional quirk that I haven't handled yet is the missing min/max for ABS_*
axes in the report descriptor. This prevents libinput from working, but other
evdev users appear to work fine. The driver in this series appears to get that
information from some vendor-specific command, and I am not sure where to hook
that up.

Regards,
Samuel

> 1: https://lwn.net/ml/linux-kernel/20180703094309.18514-1-mylene.josserand@bootlin.com/
> 2: https://github.com/alistair23/linux/commits/rM2-mainline-cyttsp5-hid
> 
> Alistair Francis (2):
>   ARM: imx_v6_v7_defconfig: Enable the cyttsp5 touchscreen
>   ARM: dts: imx7d-remarkable2: Enable the cyttsp5
> 
> Mylène Josserand (2):
>   Input: Add driver for Cypress Generation 5 touchscreen
>   dt-bindings: input: Add Cypress TT2100 touchscreen controller
> 
>  .../input/touchscreen/cypress,tt21000.yaml    |  92 ++
>  arch/arm/boot/dts/imx7d-remarkable2.dts       |  89 ++
>  arch/arm/configs/imx_v6_v7_defconfig          |   1 +
>  drivers/input/touchscreen/Kconfig             |  14 +
>  drivers/input/touchscreen/Makefile            |   1 +
>  drivers/input/touchscreen/cyttsp5.c           | 922 ++++++++++++++++++
>  6 files changed, 1119 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.yaml
>  create mode 100644 drivers/input/touchscreen/cyttsp5.c
>