| Message ID | 20211222124603.326920-1-alistair@alistair23.me (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add support for the Cypress cyttsp5 | expand |
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 >
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 > > >