linux-kernel - Re: [PATCH 2/5] Input: add driver for Focaltech FTS touchscreen

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d40faca2-fe5d-5b5a-eefe-68eb3e5e8125@gmail.com>
Date:   Sun, 12 Mar 2023 17:21:27 -0500
From:   Joel Selvaraj <joelselvaraj.oss@...il.com>
To:     Markuss Broks <markuss.broks@...il.com>,
        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>,
        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 2/5] Input: add driver for Focaltech FTS touchscreen

Hi Markuss,

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

On 12/03/23 15:40, Markuss Broks wrote:

> Why is the _ratelimited variant necessary?

I assumed in case of the interrupt working, but i2c reads fail for some
reason, it would spam a lot of error messages if the user touches the
screen continuously, like a swipe up gesture or something.

I referred to ad7879 touchscreen's irq handling code [1] and thought
it's probably best to do this, to be on the safe side. I will remove
this if it's not needed in v2.

> Overall, I think it's better to cast the data type to a struct, which
> would make this seem with less random.

Sorry, I am not sure I got this right. Do you mean I create an array of
struct called say "fts_point" that stores the x, y, type, etc. info of
all the points, then report it separately. Like similar to something
done by the auo-pixcir touchscreen driver [2]?

If I didn't get this correctly, can you show me some code in mainline,
that does it? It would be very helpful.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/touchscreen/ad7879.c?h=v6.3-rc1#n250
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/touchscreen/auo-pixcir-ts.c?h=v6.3-rc1#n162

> - Markuss
Thanks,
Joel