Hi Dmitry,
On 10/12/2023 07:53, Dmitry Torokhov wrote:
Hi Neil,
On Sat, Dec 09, 2023 at 08:33:40AM +0100, Neil Armstrong wrote:
Add initial support for the new Goodix "Berlin" touchscreen ICs.
These touchscreen ICs support SPI, I2C and I3C interface, up to
10 finger touch, stylus and gestures events.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
Support for advanced features like:
- Firmware & config update
- Stylus events
- Gestures events
- Previous revisions support (BerlinA or BerlinB)
is not included in current version.
The current support will work with currently flashed firmware
and config, and bail out if firmware or config aren't flashed yet.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Reviewed-by: Jeff LaBundy <jeff@xxxxxxxxxxx>
Signed-off-by: Neil Armstrong <neil.armstrong@xxxxxxxxxx>
Thank you for resending the patch. I think there is an issue in how you
read and parse the data in case of more than 2 fingers. It looks like in
that case you are overwriting the checksum form the first 2 and then not
reading the new checksum but use some garbage past the touch data. I
might be mistaken though...
I carefully inspected the code again, and it's correct, otherwise I would have experimented
checksum errors, which isn't the case.
First read from goodix_berlin_irq() is GOODIX_BERLIN_IRQ_READ_LEN(2) length in memory:
[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN][GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE][GOODIX_BERLIN_BYTES_PER_POINT * x]
the pre_buf_len goodix_berlin_touch_handler() get is GOODIX_BERLIN_IRQ_READ_LEN(2), the we complete the
read after the first read, but since the touch checksum is before the touch data, it works because
we complete the data.
I added some comments to clarify the memory layout and re-ordered the items
in the GOODIX_BERLIN_IRQ_READ_LEN() macro to show GOODIX_BERLIN_COOR_DATA_CHECKSUM
is before the GOODIX_BERLIN_BYTES_PER_POINT data.
I also believe you are leaking afe_data in case of success. We have the
newfangled __free(kfree) from cleanup.h that should help there.
Indeed it was leaking.
Another request - we should not have anything in goodix_berlin.h that is
not used by the I2C and SPI sub-drivers, so the only thing it should
contain is goodix_berlin_probe() declaration and dev_pm_ops. All other
defines and definitions should go to goodix_berlin_core.h.
I made a few more cosmetic changes in the attached patch, please
consider applying it.
Thanks.
Thanks,
Neil
