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

From: Markuss Broks
Date: Sun Apr 09 2023 - 09:32:39 EST


Hi Joel,

Sorry for responding late:

On 3/13/23 00:21, Joel Selvaraj wrote:
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]?

By that I meant doing something like the Zinitix driver[1] does. It has a struct data type for whatever you read from hardware, e.g.

struct point_coord {
    __le16    x;
    __le16    y;
...
};

from that driver. That way you can cast the data read to that struct and have it look a bit nicer.

This is just a suggestion though, you have the final choice in what design you choose for your code :)


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
[1]
https://elixir.free-electrons.com/linux/v6.3-rc5/source/drivers/input/touchscreen/zinitix.c


- Markuss