Re: [PATCH v12 2/4] Input: add core support for Goodix Berlin Touchscreen IC

[Neil Armstrong <neil.armstrong@linaro.org>]

On 12/12/2023 15:43, Neil Armstrong wrote:
> 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@labundy.com>
>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>
>> 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.

Ok I was wrong, the checksun is at the end, but since we check the checksum _after_
reading the missing fingers, the checksum gets read correctly and is always valid.

The first checksum check is for the header, not the finger data, so it may be
confusing.

I've added a big comment explaining what's done and how the finger data is complete
and where is the finger data checksum is all cases.

Neil

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