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

Christophe JAILLET
2023-04-15T09:55:50Z

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@public.gmane.org>
> Signed-off-by: Caleb Connolly <caleb-u60PMpPBjd35c1cvEZuMuQ@public.gmane.org>
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> 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@public.gmane.org
> S: Maintained
> F: drivers/input/joystick/fsia6b.c
>
> +FOCALTECH FTS5452 TOUCHSCREEN DRIVER
> +M: Joel Selvaraj <joelselvaraj.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> +M: Caleb Connolly <caleb-u60PMpPBjd35c1cvEZuMuQ@public.gmane.org>
> +L: linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +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@b4.vu>
> L: alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org (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;
> +} …