Date:   Sun, 12 Mar 2023 19:21:55 -0500
From:   Joel Selvaraj <joelselvaraj.oss@...il.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Caleb Connolly <caleb@...nolly.tech>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Henrik Rydberg <rydberg@...math.org>,
        Arnd Bergmann <arnd@...db.de>,
        Robert Jarzmik <robert.jarzmik@...e.fr>,
        Jeff LaBundy <jeff@...undy.com>,
        Neil Armstrong <neil.armstrong@...aro.org>,
        Markuss Broks <markuss.broks@...il.com>,
        Jean Delvare <jdelvare@...e.de>,
        Max Krummenacher <max.krummenacher@...adex.com>,
        Job Noorman <job@...rman.info>,
        Alistair Francis <alistair@...stair23.me>,
        Chris Morgan <macromorgan@...mail.com>
Cc:     linux-input@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        ~postmarketos/upstreaming@...ts.sr.ht, phone-devel@...r.kernel.org
Subject: Re: [PATCH 1/5] dt-bindings: input: touchscreen: add bindings for
 focaltech,fts

Hi Krzysztof,

Thanks for the review! I agree with most of your comments and will
fix them in v2. I have a few doubts as discussed below.

On 12/03/23 15:47, Krzysztof Kozlowski wrote:
> I have doubts you will cover here all possible FTS controllers, so
> filename should be more specific, e.g. choose the oldest device compatible.

The driver is kind of widely used and can actually support 49 touch
panel variants as per the downstream code [1]. With some slight
modifications, the other touch panels can be supported too. However, in
real world, we have only tested the driver against the two panel we have
access to (FT8719 - Poco F1 Phone and FT5452 - Shiftmq6 Phone).

Although its very generic and widely used, I agree we don't know that
will be the case forever. So I am ok with changing it to more specific
one. But I don't think the panel chip number denote which is older and
which newer. Shall I just go with focaltech,fts5452, as that's the
lowest number panel that we have tested so far and is supported?

Or do I just keep it generic as it can potentially support a lot of
variants?

>> +  focaltech,max-touch-number:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: max number of fingers supported
> 
> Why this is not implied from compatible? IOW, why this differs between
> boards?

Without proper datasheet it is kind of hard to say if this is the
maximum supported touch points by hardware or just a vendor specified
one. Because, downstream has it as devicetree property and we only know
what's set in that from each vendor tree. The FT8719 used in Poco F1
specifies 10 touch points in downstrean devicetree. But, if I specify it
as 2, it will still work fine. The FT5452 used in shiftmq6 specifies 5
touch points in downstream devicetree, but we won't know if that is the
maximum possible, unless we try to increase it upto 10 and confirm.

So, yeah without the datasheet, we will be just kind of assuming that is
is the maximum possible number of touch points by the hardware. I am not
sure if we wanna hard code that in the driver. Is it okay if we let this
configurable? Boards/Phones can use the max touch number their vendor
driver points too or if they have a datasheet, they can specify maximum
supported one too.

Kindly let me know your thoughts on this.

[1]
https://github.com/LineageOS/android_kernel_xiaomi_sdm845/blob/f1977d9edd01cff3fc9a12e09cd6a5a8052fc8ca/drivers/input/touchscreen/focaltech_touch/focaltech_config.h#L37

> Best regards,
> Krzysztof

Thanks,
Joel

Powered by blists - more mailing lists