lkml.org 
[lkml]   [2023]   [Mar]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 2/5] Input: add driver for Focaltech FTS touchscreen
From
Hi Joel,

Some comments about the driver below,

On 3/12/23 11:32, Joel Selvaraj wrote:
> 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@connolly.tech>
> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> ---
> MAINTAINERS | 8 +
> drivers/input/touchscreen/Kconfig | 12 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/focaltech_fts.c | 448 ++++++++++++++++++++++
> 4 files changed, 469 insertions(+)
> create mode 100644 drivers/input/touchscreen/focaltech_fts.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2892858cb040..deb561c356f2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7993,6 +7993,14 @@ L: linux-input@vger.kernel.org
> S: Maintained
> F: drivers/input/joystick/fsia6b.c
>
> +FOCALTECH FTS TOUCHSCREEN DRIVER
> +M: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> +M: Caleb Connolly <caleb@connolly.tech>
> +L: linux-input@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.yaml
> +F: drivers/input/touchscreen/focaltech_fts.c
> +
> FOCUSRITE SCARLETT GEN 2/3 MIXER DRIVER
> M: Geoffrey D. Bennett <g@b4.vu>
> L: alsa-devel@alsa-project.org (moderated for non-subscribers)
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 1a2049b336a6..320925bac3a1 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_FTS
> + 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.
> +
> config TOUCHSCREEN_FUJITSU
> tristate "Fujitsu serial touchscreen"
> select SERIO
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index f2fd28cc34a6..83ea2e3ce754 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -45,6 +45,7 @@ obj-$(CONFIG_TOUCHSCREEN_ELO) += elo.o
> obj-$(CONFIG_TOUCHSCREEN_EGALAX) += egalax_ts.o
> obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL) += egalax_ts_serial.o
> obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
> +obj-$(CONFIG_TOUCHSCREEN_FOCALTECH_FTS) += focaltech_fts.o
> obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
> obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
> obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
> diff --git a/drivers/input/touchscreen/focaltech_fts.c b/drivers/input/touchscreen/focaltech_fts.c
> new file mode 100644
> index 000000000000..d389c8b88944
> --- /dev/null
> +++ b/drivers/input/touchscreen/focaltech_fts.c
> @@ -0,0 +1,448 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + *
> + * FocalTech touchscreen driver.
> + *
> + * Copyright (c) 2010-2017, FocalTech Systems, Ltd., all rights reserved.
> + * Copyright (C) 2018 XiaoMi, Inc.
> + * Copyright (c) 2021 Caleb Connolly <caleb@connolly.tech>
> + * Copyright (c) 2023 Joel Selvaraj <joelselvaraj.oss@gmail.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/regmap.h>
> +#include <linux/mutex.h>
Includes must be sorted alphabetically.
> +
> +#define FTS_REG_CHIP_ID_H 0xA3
> +#define FTS_REG_CHIP_ID_L 0x9F
> +
> +#define FTS_MAX_POINTS_SUPPORT 10
> +#define FTS_ONE_TOUCH_LEN 6
> +
> +#define FTS_TOUCH_X_H_OFFSET 3
> +#define FTS_TOUCH_X_L_OFFSET 4
> +#define FTS_TOUCH_Y_H_OFFSET 5
> +#define FTS_TOUCH_Y_L_OFFSET 6
> +#define FTS_TOUCH_PRESSURE_OFFSET 7
> +#define FTS_TOUCH_AREA_OFFSET 8
> +#define FTS_TOUCH_TYPE_OFFSET 3
> +#define FTS_TOUCH_ID_OFFSET 5
> +
> +#define FTS_TOUCH_DOWN 0
> +#define FTS_TOUCH_UP 1
> +#define FTS_TOUCH_CONTACT 2
> +
> +#define FTS_DRIVER_NAME "fts-i2c"
> +#define INTERVAL_READ_REG 100 /* unit:ms */
> +#define TIMEOUT_READ_REG 2000 /* unit:ms */
Instead of using those comments, it's better to use the _MS suffix.
> +
> +#define CHIP_TYPE_5452 0x5452
> +#define CHIP_TYPE_8719 0x8719
> +
> +struct fts_ts_data {
> + struct i2c_client *client;
> + struct input_dev *input_dev;
> + struct touchscreen_properties prop;
> +
> + struct regmap *regmap;
> + int irq;
> +
> + struct regulator_bulk_data regulators[2];
> +
> + /* Touch data */
> + u8 max_touch_number;
> + u8 *point_buf;
> + int point_buf_size;
> +
> + /* DT data */
> + struct gpio_desc *reset_gpio;
> +};
> +
> +static const struct regmap_config fts_ts_i2c_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> +};
> +
> +static bool fts_chip_is_valid(struct fts_ts_data *data, u16 id)
> +{
> + if (id != CHIP_TYPE_5452 && id != CHIP_TYPE_8719)
> + return false;
> +
> + return true;
> +}
> +
> +int fts_check_status(struct fts_ts_data *data)
> +{
> + int count = 0;
> + unsigned int val, id;
> +
> + do {
> + regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
> + id = val << 8;
> + regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &val);
> + id |= val;
That register layout is surely... weird. Anyway, I'd probably do
```
        regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &id);
        regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
        id |= val << 8;
```
to make it less actions. Also, this lacks error checking for the regmap
calls.
> +
> + if (fts_chip_is_valid(data, id)) {
> + dev_dbg(&data->client->dev, "TS Ready: Chip ID = 0x%x", id);
> + return 0;
> + }
> +
> + count++;
> + msleep(INTERVAL_READ_REG);
> + } while ((count * INTERVAL_READ_REG) < TIMEOUT_READ_REG);
> +
> + return -EIO;
> +}
> +
> +static void 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;
> +
> + 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_ratelimited(&data->client->dev, "I2C read failed: %d\n",
> + error);
Why is the _ratelimited variant necessary?
> + return;
> + }
> +
> + for (i = 0; i < data->max_touch_number; i++) {
> + base = FTS_ONE_TOUCH_LEN * i;
> +
> + slot = (buf[base + FTS_TOUCH_ID_OFFSET]) >> 4;
I believe parentheses aren't needed?
> + if (slot >= data->max_touch_number)
> + break;
> +
> + x = ((buf[base + FTS_TOUCH_X_H_OFFSET] & 0x0F) << 8) +
> + (buf[base + FTS_TOUCH_X_L_OFFSET] & 0xFF);
> + y = ((buf[base + FTS_TOUCH_Y_H_OFFSET] & 0x0F) << 8) +
> + (buf[base + FTS_TOUCH_Y_L_OFFSET] & 0xFF);
> +
> + z = buf[base + FTS_TOUCH_PRESSURE_OFFSET];
> + if (z <= 0)
> + z = 0x3f;
> +
> + maj = buf[base + FTS_TOUCH_AREA_OFFSET] >> 4;
> + if (maj <= 0)
> + maj = 0x09;
> +
> + type = buf[base + FTS_TOUCH_TYPE_OFFSET] >> 6;
> +
> + input_mt_slot(input_dev, slot);
> + if (type == FTS_TOUCH_DOWN || type == FTS_TOUCH_CONTACT) {
> + input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, true);
> + touchscreen_report_pos(data->input_dev, &data->prop, x, y, true);
> + input_report_abs(input_dev, ABS_MT_PRESSURE, z);
> + input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, maj);
> + input_report_key(data->input_dev, BTN_TOUCH, 1);
> + } else {
> + input_report_key(data->input_dev, BTN_TOUCH, 0);
> + input_mt_report_slot_inactive(input_dev);
> + }
> + }
Overall, I think it's better to cast the data type to a struct, which
would make this seem with less random.
> + input_sync(input_dev);
> +}
> +
> +static irqreturn_t fts_ts_interrupt(int irq, void *dev_id)
> +{
> + struct fts_ts_data *data = dev_id;
> +
> + fts_report_touch(data);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void fts_power_off(void *d)
> +{
> + struct fts_ts_data *data = d;
> +
> + regulator_bulk_disable(ARRAY_SIZE(data->regulators),
> + data->regulators);
> +}
> +
> +static int fts_start(struct fts_ts_data *data)
> +{
> + int error;
> +
> + error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> + data->regulators);
> + if (error) {
> + dev_err(&data->client->dev, "failed to enable regulators\n");
> + return error;
> + }
> +
> + gpiod_set_value_cansleep(data->reset_gpio, 0);
> + msleep(200);
> +
> + enable_irq(data->irq);
> +
> + return 0;
> +}
> +
> +static int fts_stop(struct fts_ts_data *data)
> +{
> + disable_irq(data->irq);
> + gpiod_set_value_cansleep(data->reset_gpio, 1);
> + fts_power_off(data);
> +
> + return 0;
> +}
> +
> +static int fts_input_open(struct input_dev *dev)
> +{
> + struct fts_ts_data *data = input_get_drvdata(dev);
> + int error;
> +
> + error = fts_start(data);
> + if (error)
> + return error;
> +
> + error = fts_check_status(data);
> + if (error) {
> + dev_err(&data->client->dev, "Failed to start or unsupported chip");
> + return error;
> + }
> +
> + return 0;
> +}
> +
> +static void fts_input_close(struct input_dev *dev)
> +{
> + struct fts_ts_data *data = input_get_drvdata(dev);
> +
> + fts_stop(data);
> +}
> +
> +static int fts_input_init(struct fts_ts_data *data)
> +{
> + struct device *dev = &data->client->dev;
> + struct input_dev *input_dev;
> + int error = 0;
> +
> + input_dev = devm_input_allocate_device(dev);
> + if (!input_dev)
> + return -ENOMEM;
> +
> + data->input_dev = input_dev;
> +
> + /* Init and register Input device */
> + input_dev->name = FTS_DRIVER_NAME;
> + input_dev->id.bustype = BUS_I2C;
> + input_dev->dev.parent = dev;
> + input_dev->open = fts_input_open;
> + input_dev->close = fts_input_close;
> +
> + input_set_drvdata(input_dev, data);
> +
> + __set_bit(EV_SYN, input_dev->evbit);
> + __set_bit(EV_ABS, input_dev->evbit);
> + __set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
> +
> + input_mt_init_slots(input_dev, data->max_touch_number,
> + INPUT_MT_DIRECT);
> + input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_X);
> + input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_Y);
> + input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> + input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
> +
> + touchscreen_parse_properties(input_dev, true, &data->prop);
> + if (!data->prop.max_x || !data->prop.max_y) {
> + dev_err(dev,
> + "touchscreen-size-x and/or touchscreen-size-y not set in dts\n");
> + return -EINVAL;
> + }
> +
> + data->point_buf_size = (data->max_touch_number * FTS_ONE_TOUCH_LEN) + 3;
> + data->point_buf = devm_kzalloc(dev, data->point_buf_size, GFP_KERNEL);
> + if (!data->point_buf) {
> + dev_err(dev, "Failed to alloc memory for point buffer\n");
> + return -ENOMEM;
> + }
> +
> + error = input_register_device(input_dev);
> + if (error) {
> + dev_err(dev, "Failed to register input device\n");
> + return error;
> + }
> +
> + return 0;
> +}
> +
> +static int fts_parse_dt(struct fts_ts_data *data)
> +{
> + int error = 0;
> + struct device *dev = &data->client->dev;
> + struct device_node *np = dev->of_node;
> + u32 val;
> +
> + error = of_property_read_u32(np, "focaltech,max-touch-number", &val);
> + if (error) {
> + dev_err(dev, "Unable to read property 'focaltech,max-touch-number'");
> + return -EINVAL;
> + }
> + if (val < 2 || val > FTS_MAX_POINTS_SUPPORT) {
> + dev_err(dev, "'focaltech,max-touch-number' out of range [2, %d]",
> + FTS_MAX_POINTS_SUPPORT);
> + return -EINVAL;
> + }
> + data->max_touch_number = val;
> +
> + data->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(data->reset_gpio)) {
> + error = PTR_ERR(data->reset_gpio);
> + dev_err(dev, "Failed to request reset gpio, error %d\n", error);
> + return error;
> + }
> +
> + return 0;
> +}
> +
> +static int fts_ts_probe(struct i2c_client *client)
> +{
> + int error = 0;
> + struct fts_ts_data *data;
> +
> + 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;
> +
> + data->client = client;
> +
> + error = fts_parse_dt(data);
> + if (error)
> + return error;
> +
> + i2c_set_clientdata(client, data);
> +
> + data->regmap = devm_regmap_init_i2c(client, &fts_ts_i2c_regmap_config);
> + if (IS_ERR(data->regmap)) {
> + dev_err(&client->dev, "regmap allocation failed\n");
> + return PTR_ERR(data->regmap);
> + }
> +
> + /*
> + * 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;
> + }
> +
> + 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;
> +}
> +
> +static int fts_pm_suspend(struct device *dev)
> +{
> + struct fts_ts_data *data = dev_get_drvdata(dev);
> +
> + mutex_lock(&data->input_dev->mutex);
> +
> + if (input_device_enabled(data->input_dev))
> + fts_stop(data);
> +
> + mutex_unlock(&data->input_dev->mutex);
> +
> + return 0;
> +}
> +
> +static int fts_pm_resume(struct device *dev)
> +{
> + struct fts_ts_data *data = dev_get_drvdata(dev);
> + int error = 0;
> +
> + mutex_lock(&data->input_dev->mutex);
> +
> + if (input_device_enabled(data->input_dev))
> + error = fts_start(data);
> +
> + mutex_unlock(&data->input_dev->mutex);
> +
> + return error;
> +}
> +
> +static const struct dev_pm_ops fts_dev_pm_ops = {
> + .suspend = fts_pm_suspend,
> + .resume = fts_pm_resume,
> +};
> +
> +static const struct of_device_id fts_match_table[] = {
> + { .compatible = "focaltech,fts5452", },
> + { .compatible = "focaltech,fts8719", },
> + { /* sentinel */ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, fts_match_table);
> +
> +static struct i2c_driver fts_ts_driver = {
> + .probe_new = fts_ts_probe,
> + .driver = {
> + .name = FTS_DRIVER_NAME,
> + .pm = &fts_dev_pm_ops,
> + .of_match_table = fts_match_table,
> + },
> +};
> +module_i2c_driver(fts_ts_driver);
> +
> +MODULE_AUTHOR("Joel Selvaraj <joelselvaraj.oss@gmail.com>");
> +MODULE_AUTHOR("Caleb Connolly <caleb@connolly.tech>");
> +MODULE_DESCRIPTION("FocalTech touchscreen Driver");
> +MODULE_LICENSE("GPL");
- Markuss

\
 
 \ /
  Last update: 2023-03-27 00:55    [W:0.267 / U:0.128 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site