Re: [PATCH v4 0/4] Add support for the Cypress cyttsp5

[Alistair Francis <alistair23@gmail.com>]

On Fri, Jan 7, 2022 at 3:49 PM Samuel Holland <samuel@sholland.org> wrote:
>
> 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

Hey! That's great! PineNote support would be awesome

> 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.

Hmm... That's annoying. This series now works with the rM2 and a kobo
device, so I don't understand why it wouldn't work on the PineNote.

I just sent a v5 (sorry, I forgot to CC you) that has some code
cleanups, but I don't expect it to fix the issues you are seeing.

>
> 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.

Cool! I'm impressed you got this working!

>
> 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.

If the PineNote and eInk setups are different (they probably are if
this series isn't working for you) then there might be other changes
as well.

>
> 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.

I'll leave that for an I2C HID maintainer to answer.

>
> 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.

We will need that won't we? So then i2c_hid would have to implement
that as well?

Alistair

>
> 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
> >
>