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

From: Christophe JAILLET
Date: Sat Apr 15 2023 - 05:55:51 EST


Le 15/04/2023 à 04:02, Joel Selvaraj a écrit :
The Focaltech FTS driver supports several variants of focaltech
touchscreens found in ~2018 era smartphones including variants found on
the PocoPhone F1 and the SHIFT6mq which are already present in mainline.
This driver is loosely based on the original driver from Focaltech
but has been simplified and largely reworked.

Co-developed-by: Caleb Connolly <caleb-u60PMpPBjd35c1cvEZuMuQ@xxxxxxxxxxxxxxxx>
Signed-off-by: Caleb Connolly <caleb-u60PMpPBjd35c1cvEZuMuQ@xxxxxxxxxxxxxxxx>
Signed-off-by: Joel Selvaraj <joelselvaraj.oss-Re5JQEeQqe8AvxtiuMwx3w@xxxxxxxxxxxxxxxx>
---
MAINTAINERS | 8 +
drivers/input/touchscreen/Kconfig | 12 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/focaltech_fts5452.c | 432 ++++++++++++++++++
4 files changed, 453 insertions(+)
create mode 100644 drivers/input/touchscreen/focaltech_fts5452.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7ec4ce64f66d..1a3ea61e1f52 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8028,6 +8028,14 @@ L: linux-input-u79uwXL29TY76Z2rM5mHXA@xxxxxxxxxxxxxxxx
S: Maintained
F: drivers/input/joystick/fsia6b.c
+FOCALTECH FTS5452 TOUCHSCREEN DRIVER
+M: Joel Selvaraj <joelselvaraj.oss-Re5JQEeQqe8AvxtiuMwx3w@xxxxxxxxxxxxxxxx>
+M: Caleb Connolly <caleb-u60PMpPBjd35c1cvEZuMuQ@xxxxxxxxxxxxxxxx>
+L: linux-input-u79uwXL29TY76Z2rM5mHXA@xxxxxxxxxxxxxxxx
+S: Maintained
+F: Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml
+F: drivers/input/touchscreen/focaltech_fts5452.c
+
FOCUSRITE SCARLETT GEN 2/3 MIXER DRIVER
M: Geoffrey D. Bennett <g@xxxxx>
L: alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@xxxxxxxxxxxxxxxx (moderated for non-subscribers)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 1feecd7ed3cb..11af91504969 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -388,6 +388,18 @@ config TOUCHSCREEN_EXC3000
To compile this driver as a module, choose M here: the
module will be called exc3000.
+config TOUCHSCREEN_FOCALTECH_FTS5452
+ tristate "Focaltech FTS Touchscreen"
+ depends on I2C
+ help
+ Say Y here to enable support for I2C connected Focaltech FTS
+ based touch panels, including the 5452 and 8917 panels.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called focaltech_fts.
focaltech_fts5452?
Or should modifications be done elsewhere so that is does not look too 5452 specific?

+
config TOUCHSCREEN_FUJITSU
tristate "Fujitsu serial touchscreen"
select SERIO

[...]

+struct fts_i2c_chip_data {
+ int max_touch_points;
+};
+
+int fts_check_status(struct fts_ts_data *data)
+{
+ int error, i = 0, count = 0;

No need to init "i".

+ unsigned int val, id;
+
+ do {
+ error = regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &id);
+ if (error)
+ dev_err(&data->client->dev, "I2C read failed: %d\n", error);
+
+ error = regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
+ if (error)
+ dev_err(&data->client->dev, "I2C read failed: %d\n", error);
+
+ id |= val << 8;
+
+ for (i = 0; i < ARRAY_SIZE(fts_chip_types); i++)
+ if (id == fts_chip_types[i])
+ return 0;
+
+ count++;
+ msleep(FTS_INTERVAL_READ_REG_MS);
+ } while ((count * FTS_INTERVAL_READ_REG_MS) < FTS_TIMEOUT_READ_REG_MS);
+
+ return -EIO;
+}
+
+static int fts_report_touch(struct fts_ts_data *data)
+{
+ struct input_dev *input_dev = data->input_dev;
+ int base;
+ unsigned int x, y, z, maj;
+ u8 slot, type;
+ int error, i = 0;

No need to init "i".

+
+ u8 *buf = data->point_buf;
+
+ memset(buf, 0, data->point_buf_size);
+
+ error = regmap_bulk_read(data->regmap, 0, buf, data->point_buf_size);
+ if (error) {
+ dev_err(&data->client->dev, "I2C read failed: %d\n", error);
+ return error;
+ }
+
+ for (i = 0; i < data->max_touch_points; i++) {
+ base = FTS_ONE_TOUCH_LEN * i;

[...]

+static int fts_ts_probe(struct i2c_client *client)
+{
+ const struct i2c_device_id *id = i2c_client_get_device_id(client);
+ const struct fts_i2c_chip_data *chip_data;
+ struct fts_ts_data *data;
+ int error = 0;
+
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+ dev_err(&client->dev, "I2C not supported");
+ return -ENODEV;
+ }
+
+ if (!client->irq) {
+ dev_err(&client->dev, "No irq specified\n");
+ return -EINVAL;
+ }
+
+ data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ chip_data = device_get_match_data(&client->dev);
+ if (!chip_data)
+ chip_data = (const struct fts_i2c_chip_data *)id->driver_data;
+ if (!chip_data || !chip_data->max_touch_points) {
+ dev_err(&client->dev, "invalid or missing chip data\n");
+ return -EINVAL;
+ }
+ if (chip_data->max_touch_points > FTS_MAX_POINTS_SUPPORT) {
+ dev_err(&client->dev,
+ "invalid chip data, max_touch_points should be less than or equal to %d\n",
+ FTS_MAX_POINTS_SUPPORT);
+ return -EINVAL;
+ }
+ data->max_touch_points = chip_data->max_touch_points;
+
+ data->client = client;
+ i2c_set_clientdata(client, data);
+
+ data->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(data->reset_gpio)) {
+ error = PTR_ERR(data->reset_gpio);
+ dev_err(&client->dev, "Failed to request reset gpio, error %d\n", error);
+ return error;
+ }
+
+ data->regmap = devm_regmap_init_i2c(client, &fts_ts_i2c_regmap_config);
+ if (IS_ERR(data->regmap)) {
+ error = PTR_ERR(data->regmap);
+ dev_err(&client->dev, "regmap allocation failed, error %d\n", error);
+ return error;
+ }
+
+ /*
+ * AVDD is the analog voltage supply (2.6V to 3.3V)
+ * VDDIO is the digital voltage supply (1.8V)
+ */
+ data->regulators[0].supply = "avdd";
+ data->regulators[1].supply = "vddio";
+ error = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->regulators),
+ data->regulators);
+ if (error) {
+ dev_err(&client->dev, "Failed to get regulators %d\n", error);
+ return error;
+ }
+
+ error = devm_add_action_or_reset(&client->dev, fts_power_off, data);
+ if (error) {
+ dev_err(&client->dev, "failed to install power off handler\n");
+ return error;
+ }

Is it really needed?
This could lead to disable something that is not enabled. This looks harmless, but I wonder if it can occur?

I don't know the pm and input_dev frameworks enough to figure it myself, so this question is just about curiousity.

CJ

+
+ error = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+ fts_ts_interrupt, IRQF_ONESHOT,
+ client->name, data);
+ if (error) {
+ dev_err(&client->dev, "Failed to request IRQ: %d\n", error);
+ return error;
+ }
+
+ error = fts_input_init(data);
+ if (error)
+ return error;
+
+ return 0;
+}
[...]